Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve isolation of Che theia and che-machine-exec components #15651

Closed
skabashnyuk opened this issue Jan 10, 2020 · 13 comments
Closed

Improve isolation of Che theia and che-machine-exec components #15651

skabashnyuk opened this issue Jan 10, 2020 · 13 comments
Assignees
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. severity/P1 Has a major impact to usage or development of the system.

Comments

@skabashnyuk
Copy link
Contributor

Is your task related to a problem? Please describe.

Under some conditions, there is a possibility to reach the port of one workspace from another workspace. To improve the isolation of the major Eclipse Che components we would like to.

Describe the solution you'd like

  1. Use single pod for jwt proxy and other workspace containers.
  2. make machine-exec, theia-remote-runtime and theia endpoints listening to localhost only

Describe alternatives you've considered

n/a

Additional context

n/a

@skabashnyuk skabashnyuk added kind/task Internal things, technical debt, and to-do tasks to be performed. severity/P1 Has a major impact to usage or development of the system. team/platform labels Jan 10, 2020
@skabashnyuk skabashnyuk added this to the 7.8.0 milestone Jan 10, 2020
@amisevsk
Copy link
Contributor

How does this affect #11476? Would each pod in #11476 require a JWT container?

@skabashnyuk
Copy link
Contributor Author

How does this affect #11476? Would each pod in #11476 require a JWT container?

I don't know how it affects it. At this moment all our tooling(theia and che-machine-exec) runs in a single pod - no?

@amisevsk
Copy link
Contributor

At this moment all our tooling(theia and che-machine-exec) runs in a single pod - no?

Currently yes, but the linked issue is a plan to split workspaces into multiple pods (provided RWX volumes are available) -- some thought should be given how this change would affect future plans. If JWT proxy continues to require ~128Mi of memory, we could be looking at 300-500Mi of overhead if workspaces are split.

@skabashnyuk
Copy link
Contributor Author

@amisevsk you are right. That is a good and nice topic. However, I prefer not to mix them together because #11476 can require some time to implement it. And we would like to fix/improve what we have now already implemented.

@davidfestal
Copy link
Contributor

I would say that anyway we should at least allow the JWT proxy to be run inside the POD as a default option.

@metlos metlos self-assigned this Jan 22, 2020
@metlos metlos added the status/in-progress This issue has been taken by an engineer and is under active development. label Jan 22, 2020
@skabashnyuk skabashnyuk modified the milestones: 7.8.0, 7.9.0 Jan 29, 2020
@skabashnyuk skabashnyuk assigned metlos and mshaposhnik and unassigned metlos Jan 29, 2020
@metlos
Copy link
Contributor

metlos commented Feb 10, 2020

I could make this work by moving the jwtproxy to the workspace pod and make it proxy the secure servers by resending the traffic to 127.0.0.1 and appropriate secure server port.

While this works, it has at least two consequences:

  • doesn't prevent the supposedly secure servers from listening on 0.0.0.0 or any other interface/IP address which would make them accept traffic not proxied by jwtproxy
  • The secure servers need to listen on 127.0.0.1 which has not been the requirement so far. In 7.8.0 and prior, a secure server merely needs to listen on the pod IP address.

This is solvable in two ways IMHO:

  1. We just document that if an endpoint is secure: true, the backing server needs to listen solely on 127.0.0.1 and listening on any more IP addresses essentially enables unsecured access to the backing server.
  2. We add a new attribute to the endpoint, something like private: true which tells Che to proxy access to 127.0.0.1. If such attribute was false, Che would deploy a service for the backing server and put a jwtproxy in front of that service, just like it does at the moment. Having private: false leaves the backing server open to unsecured access, which would have to be documented. We'd keep the current behavior though, so plugins/devfiles could opt in to this feature gradually.

@skabashnyuk @sleshchenko @l0rd WDYT?

@sleshchenko
Copy link
Member

We add a new attribute to the endpoint, something like private: true which tells Che to proxy access to 127.0.0.1.

private: true does not solve an issue you described with processes which listen to 0.0.0.0

doesn't prevent the supposedly secure servers from listening on 0.0.0.0 or any other interface/IP address which would make them accept traffic not proxied by jwtproxy

right?

we add a new attribute to the endpoint, something like private: true

It's a bit confusing to have public and private attibutes at the same time, like

  attributes:
    public: true // I need URL for this endpoint
    secure: true // I need to make sure that nobody accesses my server without token with URL
    privates: true // I need to make sure that nobody accesses my server without token withing the cluster

So, my personal +1 for

We just document that if an endpoint is secure: true, the backing server needs to listen solely on 127.0.0.1 and listening on any more IP addresses essentially enables unsecured access to the backing server.

in case private: true does not help to solve issues with processes which listen to 0.0.0.0....

@l0rd
Copy link
Contributor

l0rd commented Feb 13, 2020

I am +1 for solution n.1

@metlos
Copy link
Contributor

metlos commented Feb 15, 2020

@metlos metlos closed this as completed Feb 15, 2020
@metlos
Copy link
Contributor

metlos commented Feb 15, 2020

Note that the PR in che-docs that clarifies the assumptions about the secure servers is still open: eclipse-che/che-docs#1075

@nickboldt
Copy link
Contributor

Based on discussion today, current status is:

  • fixed/working for multiuser mode, will be in 7.9.0 release.
  • broken/ not working for singleuser mode, which does not impact CRW 2.1 (no singleuser mode available)
  • Once 7.9.0 is live, fix will be reverted in master and a more complete solution will be done to support both single and multiuser modes in Che 7.10, with backport (if maintenance release needed) to 7.9.1 and included in CRW2.1

@nickboldt
Copy link
Contributor

This issue is "closed" and has label "in progress".

So I'm reopening it.

@metlos
Copy link
Contributor

metlos commented Mar 6, 2020

#16053 is implemented so this is now complete IMHO.

@metlos metlos closed this as completed Mar 6, 2020
@metlos metlos removed the status/in-progress This issue has been taken by an engineer and is under active development. label Mar 6, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

8 participants