Skip to content

Conversation

@l0rd
Copy link

@l0rd l0rd commented Jan 20, 2020

This is a first draft of the Cloud Shell proposal. cc @deads2k @davidfestal

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 20, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: l0rd
To complete the pull request process, please assign shawn-hurley
You can assign the PR to them by writing /assign @shawn-hurley in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@l0rd
Copy link
Author

l0rd commented Jan 20, 2020

/assign @shawn-hurley

@l0rd
Copy link
Author

l0rd commented Jan 30, 2020

cc @spadgett

@l0rd
Copy link
Author

l0rd commented Jan 30, 2020

/assign @spadgett @benjaminapetersen


![](https://i.imgur.com/XWcgHeI.png)

The endpoint is secured through an [OpenShift OAuth Proxy](https://github.com/openshift/oauth-proxy) sidecar. The client includes the user OpenShift token in its requests HTTP header. Only authenticated OpenShift users that satisfy the RBAC policy requirements are able to send requests to the Terminal server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you have the ability to indicate that only userX can exec or attach into PodA, why do you need to separately authenticate them. Can't you just use a normal exec call?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the web terminal we use has two API methods:

  • connect - initialize exec to k8s API
  • attach - connect to initialized exec.

This design was made from the beginning on Che and it allows (but not sure if it's implemented, maybe for Theia) to restore the terminal state after user refreshes a page.

But as an alternative - we could leave the only one method which would do both operations, it would allow avoiding the need to protect our endpoints with authentication.

But then the client should be somehow able to get k8s client.
In case, of OpenShift OAuth Proxy - it does it for CloudShell: use OpenShiftOAuth to get scoped token, store it in the cookies, and it let CloudShell client just send a request and authorization header will be provided by browser.

So, I hope it makes the current implementation a bit cleaner but we're able to discuss and find other possible implementations.


#### Users Token Injection in Tooling Containers

That's something that we still need to figure out (see "Open Questions" section) but the injection of the user token cannot be achieved through a secret because cluster editors would be able to access it. Options are 1) xtermjs that automatically execute a command at startup 2) the `DevWorkspaces` controller that copies the token in the tooling container after it has been started.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you control the image at the other end, when you exec in, the first thing you could do is copy the user token into the pod and create a kubeconfig file and KUBECONFIG env var

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first thing you could do is copy the user token into the pod and create a kubeconfig file and KUBECONFIG env var

Yeah, we should be able to implement something like that.
The question is not addressed yet - where to get user's personal token.
Currently, OpenShift OAuth proxy uses SA and is not able to request user's personal token, but only some of the existing roles
https://docs.openshift.com/container-platform/3.5/architecture/additional_concepts/authentication.html#service-accounts-as-oauth-clients

So, we need to take a look if we are able to register a dedicated OAuth client for CloudShell, OpenShift OAuth proxy could use it - our server side is able to get user's personal token and initialize kubeconfig. Will play with it shortly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there seem to be an issue with OAuth Client for CloudShell, I've described it here eclipse-che/che#16515

@deads2k Maybe you have a recommendation on how we could get user's personal token in a safe manner?
During the call, @davidfestal shared that he wondered if it's possible to have some proxy on OpenShift Console side and the URL is available for WorkspaceController...
Then CloudShell would be requested from OpenShift Console domain and we'd be able to reuse the same token OpenShift Console does.


#### Cloud Shell Pod Exec Denial Policy

Users tokens are injected in the Tooling container. That's required for `oc` or `odo` to work properly. As a consequence access to the Tooling container should be allowed **only to the user of the Cloud Shell instace**. That's something that the authorization mechanism based on RBAC seen above is not able to garantee: users with editor privileges at cluster scope will be able to exec into the Pod.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line breaks every 120 char or every sentence to make comments eeasier


#### Cloud Shell Pod Exec Denial Policy

Users tokens are injected in the Tooling container. That's required for `oc` or `odo` to work properly. As a consequence access to the Tooling container should be allowed **only to the user of the Cloud Shell instace**. That's something that the authorization mechanism based on RBAC seen above is not able to garantee: users with editor privileges at cluster scope will be able to exec into the Pod.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe the API interaction you will use to guarantee this? Will it take advantage of certain aspects of pod immutability? It cannot be an annotation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is

  • A Mutating Admission Webhook that adds an annotation with the creator UID to the DevWorkspace CR at creation time;

Then WorkspaceController creates Deployment with that creator annotation in metadata.annotations + spec.template.metadata.annotations. As a result, deployment creates pods with creator annotation propagated.

  • A Mutating Admission Webhook that validates that the creator annotation is not modified in workspace-related deployments and pods. If the value is omit - it's restored from the previous object state, if it's modified - error is thrown.

  • A Validating Admission Webhook that validates exec to make sure that the user who request connect to the workspace-related pod is the same as pod creator annotation.

Hope it answers: Can you describe the API interaction you will use to guarantee this?
If not - let me know.

Will it take advantage of certain aspects of pod immutability?

Do you mean that it would be better to propagate creator info like through Env vars, or command/args, or other fields which is immutable?
We haven't thought about it before. Will take into account.


- A Mutating Admission Webhook that adds a user attribute to the DevWorkspace CR at creation time
- A Validating Admission Webhook that block changes to the user attribute of a DevWorkspace CR
- A Validating Admission Webhook that block changes to the Cloud Shell Pod annotation that stores the reference to the DevWorkspace CR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we simplify down to a least common denominator of a user reference? A webhook on pods could actually protect an annotation (making it set-once) indicating user and then prevent other execs. It wouldn't preclude you embedding it in another resource type, but it makes your footprint on kube smaller.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean something like:

  • the only Workspace CR has creator annotation;
  • deny modifing OwnerReference for deployment and pods;
  • no need to validate deployment or pods with webhooks;
  • ValidatingWebhook which check if it's try to connect to a workspace-related pod, if yes - finds to which workspace CR instance this pod belongs to, use creator from there.

If yes - we considered this option but it would make validating webhook check slower, and since we're not able to filter CreateExecOptions with object/label selectors - it would mean that every exec would be slower.

Please elaborate more if you meant something else.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 4, 2020
@l0rd l0rd changed the title Cloud Shell Web Terminal Oct 5, 2020
@l0rd
Copy link
Author

l0rd commented Oct 5, 2020

It has been implemented as web-terminal operator.

A blog post has been published on the OpenShift blog.

Closing this PR.

@l0rd l0rd closed this Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants