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

The username (string) should be admitted in AppDefinition for container creation #43

Open
dmm9 opened this issue Sep 11, 2023 · 4 comments

Comments

@dmm9
Copy link

dmm9 commented Sep 11, 2023

It should be possible to start containers with a username as string instead of user id (integer).
See:
https://github.com/eclipsesource/theia-cloud-helm/blob/2c7fe9eb083c4223fafafea1967df3924a4d5088/charts/theia-cloud-crds/templates/appdefinition-spec-resource.yaml#L42C19-L42C32

In some cases, (theia) images are created with e.g. RUN adduser theia, leaving the responsibility of assigning a UID to the underlying OS. Although uncommon, it could also be the case that this is not the first user that is created. So, the Custom Resource should accept the username as parameter to create the resources.

I have had the case where user 'theia' was assigned the UID 104 during image build for some reason I do not know. And 'messagebus' was UID 101. Theia Cloud initiated the workspace with uid ('messagebus'), but the workspace owner was 'theia'. As a user, it was impossible to modify any file in the workspace due to "Permission denied". The workaround is to force selection of UID 101 in the workspace dockerfile by using RUN adduser --system --uid 101 --group theia, because passing the username 'theia' to Theia Cloud is currently not possible.

@jfaltermeier
Copy link
Contributor

The UID to use can be set via the AppDefinition: https://github.com/eclipsesource/theia-cloud-helm/blob/2c7fe9eb083c4223fafafea1967df3924a4d5088/charts/theia-cloud-crds/templates/appdefinition-spec-resource.yaml#L41 Since you specify your docker image there as well you can align the UID with the user that works for your image.

The reason why we don't use the name but the id is because the value is used to configure the pod's security context on kubernetes side: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#podsecuritycontext-v1-core This asks for an integer

@dmm9
Copy link
Author

dmm9 commented Sep 12, 2023

According to the docs in your link:

runAsUser (integer) - The UID to run the entrypoint of the container process. Defaults to user specified in image metadata if unspecified. May also be set in SecurityContext. If set in both SecurityContext and PodSecurityContext, the value specified in SecurityContext takes precedence for that container. Note that this field cannot be set when spec.os.name is windows.

So it could be left unspecified and it would always start as the user specified in the workspace image. This is even more straightforward as specifying the username in the AppDefinition. Note that also runAsGroup should be left empty in this case.

To enforce good practices and check for non-root containers, the parameter runAsNonRoot could be added.

runAsNonRoot (boolean) - Indicates that the container must run as a non-root user. If true, the Kubelet will validate the image at runtime to ensure that it does not run as UID 0 (root) and fail to start the container if it does. If unset or false, no such validation will be performed. May also be set in SecurityContext. If set in both SecurityContext and PodSecurityContext, the value specified in SecurityContext takes precedence.

@dmm9
Copy link
Author

dmm9 commented Sep 12, 2023

So, my proposal would be:

  • Alternative 1:
    • Remove uid from AppDefinition spec
    • When starting the pod, do not set runAsUser and add runAsNonRoot
  • Alternative 2:
    • Keep uid in AppDefinition spec
    • Check if uid is set in the AppDefinition. If not, do not include 'runAsUser' when creating the pods.
    • Always add 'runAsNonRoot' when starting the pods.

@jfaltermeier
Copy link
Contributor

Our current convention is to also use the uid as the gid and fsGroup. fsGroup is used for all pods in the container.
So rw-access on persisted storage is something that would have to be checked as well, when we make changes here.
Please open a new feature request here: https://github.com/eclipsesource/theia-cloud/issues/new/choose
Contributions are welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants