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

Specify the workspace Pods securityContext (SETGID and SETUID) when developers have container build capabilities #21770

Closed
l0rd opened this issue Oct 13, 2022 · 12 comments
Assignees
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.

Comments

@l0rd
Copy link
Contributor

l0rd commented Oct 13, 2022

Is your enhancement related to a problem? Please describe

Even if the container build is enabled in the CheCluster CR, the required build capabilities (SETUID, SETGID and "allowPrivilegeEscalation": true) are not explicitly requested in the workspaces Pod SecurityContext.

This is not an issue if the container-build SCC is the only SCC of the workspaces service accounts.

But, if the workspace ServiceAccount is associated with multiple SCCs, the capabilities of the SCC that requires less privileges are picked. And the developer container won't have SETUID and SETGID capabilities and trying to run a container build will fail.

Describe the solution you'd like

The DevWorkspace Operator provides a way to specify Pods security context using a DevWorkspaceOperatorConfiguration (DWOC) CR. When the build capabilities are enabled, Che Operator should apply a DWOC with the security context {"securityContext":{"capabilities":{"add": ["SETGID", "SETUID"]}, "allowPrivilegeEscalation": true}}).

Additional context

This was initially considered a DWO issue (devfile/devworkspace-operator#884) but the DWO already exposes a mechanism to specify Pods SecurityContext (with the DWOC) so that issue has been closed.

The corresponding OpenShift Dev Spaces issue.

In the future we may allow administrators to specify a custom security context in the CheCluster:

spec:
  devEnvironments:
    podsSecurityContext: {"capabilities":{"add": ["SETGID", "SETUID"]}, "allowPrivilegeEscalation": true}

but that's a separate issue.

@l0rd l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Oct 13, 2022
@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 Oct 13, 2022
@l0rd l0rd added severity/P1 Has a major impact to usage or development of the system. area/devworkspace-che-operator and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Oct 13, 2022
@AObuchow
Copy link

From what I understand, this issue is about propagating containerOverrides and podOverrides from the Che Cluster CR into the Che-owned DWOC and then having DWO inject these overrides into devworkspaces.

Currently, containerOverrides and podOverrides must be specified from an attribute in the devworkspace. There is no DWOC field to automatically inject container and pod overrides into devworkspaces, though this could probably be added (@amisevsk let me know if you have any thoughts on this topic).

However, in the example given for this issue, the pod and container overrides are being used to specify the workspace pod and container security contexts. Setting the pod security context for workspaces in the DWOC is already possible and setting the container security context for workspaces in the DWOC will soon be possible.

Thus, an alternate solution could be to allow setting the pod and container security contexts from the Che Cluster CR, which then are propagated to the Che-owned DWOC, and used by DWO.
This could look something like the following in the Che Cluster spec:

spec:
  devEnvironments:
    containerBuildConfiguration:
       containerSecurityContext: {"capabilities":{"add": ["SETGID", "SETUID"]}}
       podSecurityContext:  {"allowPrivilegeEscalation": false}

@l0rd @ibuziuk let me know if you have any thoughts on this alternate proposal.

@ibuziuk
Copy link
Member

ibuziuk commented Oct 20, 2022

@AObuchow the proposal absolutely makes sense e.g. security context is set on Eclipse Che CR level and propagated to the DWOC without the dashboard being involved. However if I remember correctly podSecurityContext is currently ignored on OpenShift cc: @amisevsk

@amisevsk
Copy link
Contributor

However if I remember correctly podSecurityContext is currently ignored on OpenShift

PR devfile/devworkspace-operator#953 adds the containerSecurityContext config field and also makes DWO take podSecurityContext into account when running on OpenShift :)

Note another potentially simpler approach, if our specific goal is supporting containerBuildCapability -- we don't yet need to have CheCluster fields for pod/container securityContext:

Step 1

Add CheCluster functionality to provision "known, good" container and pod security context into Che-owned DevWorkspaceOperatorConfig when containerBuildCapability: true. This basically automates what we would have to document in a "enabling container build in Che" docs:

CheCluster:

spec:
  devEnvironments:
    disableContainerBuildCapabilities: false # Enable container build

...causes Che Operator to add fields to DevWorkspaceOperatorConfig:

config:
  workspace:
    # Whatever Che Operator decides is necessary here, maybe even a full config
    containerSecurityContext: {"capabilities":{"add": ["SETGID", "SETUID"]}}
    podSecurityContext: {"allowPrivilegeEscalation": false}

This provides a starting point for enabling container builds by simply toggling one field in CheCluster

Step 2

What Andrew is suggesting -- add CheCluster fields to allow further customizing securityContext:

spec:
  devEnvironments:
    disableContainerBuildCapabilities: true/false
    containerSecurityContext: <securityContext>
    podSecurityContext: <podSecurityContext>

Setting containerSecurityContext updates the security contexts propagated to the DWOC and allows for finer-grained configuration in situations where it's necessary, but 90%+ of users ideally don't need to set these fields.

@AObuchow
Copy link

AObuchow commented Oct 20, 2022

Another thing I'd like to have clarified @l0rd :

podSecurityContext:  {"allowPrivilegeEscalation": false}

Pod Security Context does not have the allowPrivilegeEscalation field. This field exists in the Security Context which gets applied to the container.

So what should be the default Pod Security Context to enable container builds? Perhaps I misunderstood something.

@l0rd
Copy link
Contributor Author

l0rd commented Oct 20, 2022

Pod Security Context does not have the allowPrivilegeEscalation field. This field exists in the Security Context which gets applied to the container.

Right. That was a mistake. I also don't think that setting allowPrivilegeEscalation: true is necessary to enable containers build (SETGID and SETUID should be enough).

Thus, an alternate solution could be to allow setting the pod and container security contexts from the Che Cluster CR, which then are propagated to the Che-owned DWOC, and used by DWO.

Being able to use pods override may be used to enable kata containers (specifying podSpec.runtimeClassName: kata) and is consistent with the Devfile API (I don't have to remember 2 distinct syntax). So I would prefer that. Maybe adding container and pod overrides in the DWOC. But we need to keep things simple too so I am ok with your approach if that's significantly simpler to implement.

we don't yet need to have CheCluster fields for pod/container securityContext

Agreed. Let's do that in 2 steps.

@AObuchow
Copy link

@l0rd Thank you for the clarification & feedback :)

For now, I think it makes sense for me to follow step 1 of @amisevsk's proposal (i.e. configure the DWOC workspace security contexts based on the value of disableContainerBuildCapabilities) to enable container builds. Then as a future step, we can decide if adding container and pod overrides to the DWOC makes sense or if the dashboard should inject these override values (to allow specifying pod and container overrides from the Che Cluster CR).

@l0rd
Copy link
Contributor Author

l0rd commented Oct 25, 2022

I have done some tests and:

  • The filed allowPrivilegeEscalation: true in the container security context is required (otherwise builds fail).
  • The annotation openshift.io/scc: container-build in the Pod is not required and I believe we can close the issue to add the attribute on the DevWorkspace: the DevWorkspace operator should not need that.

@amisevsk
Copy link
Contributor

amisevsk commented Oct 26, 2022

The annotation openshift.io/scc: container-build is added by OpenShift itself to annotate which SCC it resolved for the pod -- it should be present if the correct SCC is being used but setting it ourselves does nothing (it's overwritten by whatever OpenShift decides to use based on priority).

@AObuchow
Copy link

The annotation openshift.io/scc: container-build in the Pod is not required and I believe we can close #21756: the DevWorkspace operator should not need that.

@l0rd As mentioned by @amisevsk in the above comment, the openshift.io/scc: container-build annotation is managed by OpenShift, but it is possible the controller.devfile.io/scc: container-build attribute may still be required for the devworkspace (I haven't verified this yet though)

@amisevsk
Copy link
Contributor

The attribute is required, as it instructs DWO to grant the workspace's serviceaccount permissions to use the SCC; otherwise pod creation will fail.

@AObuchow
Copy link

@l0rd last clarification (hopefully 😉):
Is it enough to have the Container Security Context configured for the workspace? Or is configuring the Pod Security Context required?

In my testing, the container-build SCC was correctly used with the Container Security Context configured and the Pod Security Context left empty.

AObuchow added a commit to AObuchow/che-operator that referenced this issue Oct 26, 2022
@l0rd
Copy link
Contributor Author

l0rd commented Oct 27, 2022

@AObuchow @amisevsk sorry for the confusion about the SCC attribute in the devfile, I understand now why this is still required.
@AObuchow yes the container SecurityContext is enough. I have modified this issue description to reflect that.

AObuchow added a commit to AObuchow/che-operator that referenced this issue Oct 27, 2022
AObuchow added a commit to AObuchow/che-operator that referenced this issue Oct 27, 2022
@ibuziuk ibuziuk mentioned this issue Nov 2, 2022
73 tasks
AObuchow added a commit to AObuchow/che-operator that referenced this issue Nov 8, 2022
AObuchow added a commit to AObuchow/che-operator that referenced this issue Nov 8, 2022
AObuchow added a commit to AObuchow/che-operator that referenced this issue Nov 10, 2022
@l0rd l0rd changed the title Configure DevWorkspace podOverrides and containerOverrides if containerBuildCapability is enabled Specify the workspace Pods securityContext (SETGID and SETUID) when developers have container build capabilities Nov 18, 2022
dkwon17 pushed a commit to dkwon17/che-operator that referenced this issue Nov 18, 2022
@ibuziuk ibuziuk mentioned this issue Nov 22, 2022
68 tasks
AObuchow added a commit to AObuchow/che-operator that referenced this issue Dec 7, 2022
AObuchow added a commit to AObuchow/che-operator that referenced this issue Dec 7, 2022
@ibuziuk ibuziuk mentioned this issue Dec 13, 2022
82 tasks
tolusha pushed a commit to AObuchow/che-operator that referenced this issue Dec 22, 2022
AObuchow added a commit to AObuchow/che-operator that referenced this issue Dec 23, 2022
amisevsk added a commit to amisevsk/che-operator that referenced this issue Jan 26, 2023
Previously-applied workaround is no longer necessary, as SCC selection
was fixed with eclipse-che/che#21770.

Signed-off-by: Angel Misevski <[email protected]>
AObuchow added a commit to AObuchow/che-operator that referenced this issue Jan 26, 2023
AObuchow added a commit to AObuchow/che-operator that referenced this issue Jan 26, 2023
nickboldt pushed a commit to eclipse-che/che-operator that referenced this issue Jan 30, 2023
* feat: configure workspace security context for container builds

Fix eclipse-che/che#21770

Signed-off-by: Andrew Obuchowicz <[email protected]>

* Set SCC allowPrivilegeEscalation to true when container build enabled (#1596)

* Set SCC allowPrivilegeEscalation to true when container build enabled

Running Podman inside a container in OpenShift requires the pod to have
allowPrivilegeEscalation: true in its security context.

* Fix tests

Signed-off-by: Angel Misevski <[email protected]>

* fix: set scc priority to null

Signed-off-by: Anatolii Bazko <[email protected]>

---------

Signed-off-by: Andrew Obuchowicz <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
Co-authored-by: Angel Misevski <[email protected]>
Co-authored-by: Anatolii Bazko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. severity/P1 Has a major impact to usage or development of the system.
Projects
None yet
Development

No branches or pull requests

5 participants