Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[DevWorkspace] Make it possible to bind namespace role to a namespace service-account #20749

Closed
vinokurig opened this issue Nov 9, 2021 · 11 comments
Labels
engine/devworkspace Issues related to Che configured to use the devworkspace controller as workspace engine. 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. sprint/current

Comments

@vinokurig
Copy link
Contributor

Is your task related to a problem? Please describe

When provisioning a namespace, Che sets serviceAccountName as che-workspace, but in the DevWorkspace installation we have another Service Account like workspace<workspace id>-sa which is actually used for a workspace. So when creating a role binding it is binded to the che-workspace service account, but when I make a kubernetes request from a workspace container it is using a token of the workspace<workspace id>-sa service account.

Describe the solution you'd like

Need to have a way to bind namespace roles to a namespace service-account.

Describe alternatives you've considered

No response

Additional context

No response

@vinokurig vinokurig added kind/task Internal things, technical debt, and to-do tasks to be performed. engine/devworkspace Issues related to Che configured to use the devworkspace controller as workspace engine. labels Nov 9, 2021
@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Nov 9, 2021
@mshaposhnik mshaposhnik added team/controller severity/P2 Has a minor but important impact to the usage or development of the system. and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Nov 9, 2021
@svor svor added severity/P1 Has a major impact to usage or development of the system. and removed severity/P2 Has a minor but important impact to the usage or development of the system. labels Nov 10, 2021
@svor
Copy link
Contributor

svor commented Nov 10, 2021

It blocks us to persist Che-Theia preferences when DW is enabled #20622
@ibuziuk @amisevsk is it possible to take this issue into the next sprint?
cc @l0rd

@amisevsk
Copy link
Contributor

The DevWorkspace Operator has to be limited in what permissions it offers to the workspace service account -- any permissions the workspace SA gets can be used by anyone with DevWorkspace RBAC permissions, so there's a risk of privilege escalation.

Currently, the RBAC provisioned for workspace ServiceAccounts is shown here. This role+rolebinding supercedes any role/rolebinding created by Che. The workspace role already allows for get/create/delete for the workspace-credentials-secret, as discussed in devfile/devworkspace-operator#483 to ultimately support #19837.

We can extend this role but I would like to be careful to extend it minimally while supporting as wide a set of use-cases as possible. To this end, there are 3 options I see:

  1. Add workspace-preferences-secret to the DW SA default role. This would resolve this issue immediately, but it's not clear why workspaces would need access to two specific secrets (everything could ostensibly stored in one, under separate keys).
  2. Modify Theia and other editors running in DevWorkspaces to use one secret for everything (workspace-credentials-secret). This is also not ideal since we would be using a secret named credentials to store preferences.
  3. Add a permission to access a specific configmap in the default DW SA role (e.g. workspace-preferences-configmap). This makes sense from an API perspective (configmaps are meant to store configuration/preferences) but might require work on the Theia side.

I'm happy to do any of the above options, but I also don't want to commit to a path without discussion. Option 1 likely makes the most sense from a backwards compatibility perspective, as Theia is already using workspace-preferences-secret in the non-DW case, but Option 3 makes more sense to me from a tidiness perspective (workspaces can access one secret and one configmap).

@vinokurig
Copy link
Contributor Author

@amisevsk

Add workspace-preferences-secret to the DW SA default role. This would resolve this issue immediately, but it's not clear why workspaces would need access to two specific secrets (everything could ostensibly stored in one, under separate keys).

what exactly two secrets do you mean by two specific secrets?

Modify Theia and other editors running in DevWorkspaces to use one secret for everything (workspace-credentials-secret). This is also not ideal since we would be using a secret named credentials to store preferences.

To use secrets from editors we need an access to them. In DW workspace we use token from DW servie-account which is not binded to the secretes role.

Add a permission to access a specific configmap in the default DW SA role (e.g. workspace-preferences-configmap). This makes sense from an API perspective (configmaps are meant to store configuration/preferences) but might require work on the Theia side.

We need to have permissions for both service accounts, for dev-workspace SA (workspace<workspace id>-sa) and usual (che-worksace) SA

@amisevsk
Copy link
Contributor

amisevsk commented Nov 15, 2021

The che-workspace SA is ignored when using DevWorkspaces, since only one SA can be attached to a pod. The DevWorkspace Operator creates the workspace<workspace-id>-sa serviceaccount with permissions specified above. This SA allows access to the workspace-credentials-secret already:

# Within workspace pod
$ curl --cacert /var/run/secrets/kubernetes.io/serviceaccount/ca.crt \
  -H "Authorization: Bearer $(cat /var/run/secrets/kubernetes.io/serviceaccount/token)" \
  "https://kubernetes.default/api/v1/namespaces/$(cat /var/run/secrets/kubernetes.io/serviceaccount/namespace)/secrets/workspace-credentials-secret"

{
  "kind": "Secret",
  "apiVersion": "v1",
  "metadata": {
  [...]
}

If no other secret access is required, then DWO already provides all access needed.

what exactly two secrets do you mean by two specific secrets?

PR eclipse-che/che-server#174 attempts to add privileges to access an additional secret, workspace-preferences-secret. This is not in the DWO RBAC rules for DevWorkspace ServiceAccounts. We can add it (option 1 above), or Theia could reuse the workspace-credentials-secret (option 2), or it could be reworked to use a configmap and we could add permissions to access a configmap in DWO (option 3).

Before committing to extending the workspace SA with additional privileges, I'd like to discuss the options and make sure the path chosen makes sense going forward (e.g. I think having access to a credentials secret and a preferences configmap will be more useful in the future than two specific secrets).

@max-cx
Copy link

max-cx commented Nov 15, 2021

Hi, a question to the assignee of this issue:

Will the outcome require any changes to the relevant content of the Installation Guide or Administration Guide or End-user Guide?

Yes/No?

@vinokurig
Copy link
Contributor Author

@amisevsk

If no other secret access is required, then DWO already provides all access needed.

We need to have patch permissions for the workspace-credentials-secret in the workspace role. Then adding the workspace-preferences-secret as a resource name to the workspace role should be enough to solve the issue.

@amisevsk
Copy link
Contributor

The question I'm asking is whether we should add workspace-preferences-secret to the workspace role in the first place though -- once it's added, it's harder to remove, so I want to make sure this makes sense as opposed to e.g. adding get and patch for a specific configmap instead.

In other words, as an abstract user of DevWorkspaces, would I prefer having access to two secrets, named workspace-credentials-secret and workspace-preferences-secret, or would I prefer having access to a secret workspace-credentials-secret and a configmap workspace-preferences-configmap. IMO the latter makes more sense.

@JPinkney
Copy link
Contributor

Is there a point of having workspace preferences be a secret other then the fact that there is some theia code that makes it easier to interact with secrets?

If I were a random user of devworkspace IMO I would expect workspace preferences to be in a configmap rather then a secret

@vinokurig
Copy link
Contributor Author

There is no technical reason for storing preferences in secret but not in config-map, so I am +1 to have a config-map workspace-preferences-configmap.

@amisevsk
Copy link
Contributor

@vinokurig Great! PR devfile/devworkspace-operator#675 should add the required permissions; it will be included in the v0.11.0 release of DWO.

@amisevsk
Copy link
Contributor

Closing as devfile/devworkspace-operator#675 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine/devworkspace Issues related to Che configured to use the devworkspace controller as workspace engine. 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. sprint/current
Projects
None yet
Development

No branches or pull requests

8 participants