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

feat: define user id and group instead of name in the Dockerfile #79

Closed
wants to merge 1 commit into from
Closed

feat: define user id and group instead of name in the Dockerfile #79

wants to merge 1 commit into from

Conversation

camilamacedo86
Copy link

@camilamacedo86 camilamacedo86 commented Jul 9, 2022

Description

Define user id and group instead of name in the Dockerfile
Note that the entry point will set the username.

Motivation

Be able to use this image in Pods qualified with security context restricted in Kubernetes and Opensift clusters.

TL'DR:

  • In a k8s cluster if we try to run a Pod/Container with this image as restricted without informing RunAsUser we will face the error: "container has runAsNonRoot and image will run as root …
  • Then, if we try to use this image in Openshift in the same circumstances we will check:
    Error: container has runAsNonRoot and image has non-numeric user (memcache), cannot verify user is non-root Therefore, for a Pod/Container to be qualified to the restricted-v2 SCC (the most restricted context) we cannot use RunAsUser.

NOTE: It was checked locally with the command memcached -m 1024 -o modern -v and all worked fine.

@camilamacedo86
Copy link
Author

camilamacedo86 commented Jul 9, 2022

@tianon @yosifkit @blar @ncopa @michael-k
Could we please get this one merged and do a new release?
Could you give a hand on this one?

@tianon
Copy link
Member

tianon commented Jul 11, 2022

Thanks for the contribution!

I'd honestly rather not maintain this UID explicitly like this, especially since there's no functional difference between the two when actually running the image (and one is distinctly more user-friendly to read than the other). 😬

There are a few options here that don't require us to make/maintain any changes to the images:

  • run the image explicitly as the user (or as any user -- it really doesn't matter which user you run this image as, and you could even choose a different one randomly every time it starts since there's no on-disk state which is what usually makes UID/GID changes difficult)

  • maintain a short Dockerfile that's auto-built with just FROM memcached \n USER 11211:11211 (if it has to be baked into the image itself)

  • implement something like a mutating admission webhook on the cluster which can pull the image and resolve the UID/GID appropriately to then dynamically update the applied configuration appropriately (which I think is the most "Kubernetes" solution to this problem)

@camilamacedo86
Copy link
Author

camilamacedo86 commented Jul 11, 2022

Hi @tianon,

Thank you for the reply. Following some comments inline. I hope that the following info can show you why this silly, nit change is important and it can persuade you to support this change and help us out :-)

I'd honestly rather not maintain this UID explicitly like this,

We do this in the image already, see:

RUN addgroup -g 11211 memcache && adduser -D -u 11211 -G memcache memcache

especially since there's no functional difference between the two when actually running the image

That is not true.

From Docker Docs

Note that when specifying a group for the user, the user will have only the specified group membership. Any other configured group memberships will be ignored.

When the user doesn’t have a primary group then the image (or the next instructions) will be run with the root group.

From: https://docs.docker.com/engine/reference/builder/#user

NOTE: Therefore because the container user is always a member of the root group, the container user can read and write these files.

From Dockefile best practices

Screenshot 2022-07-12 at 00 10 10

https://docs.docker.com/develop/develop-images/dockerfile_best-practices/

On Kubernetes (>= 1.24) you can check

If we create a Pod/Container to be admitted as restricted and does not violate the PodSecurity with the following security context config:

spec:
      securityContext:
        runAsNonRoot: true
        seccompProfile:
          type: RuntimeDefault
      ...
      containers:
      - name: controller-manager
        securityContext:
          allowPrivilegeEscalation: false
          capabilities:
            drop:
            - ALL

The Pod/Container will not run and the error container has runAsNonRoot and image will run as root … will be raised.

On Openshift

To allow a workload to run as restricted(restricted-v2 SCC) we MUST leave the runAsUser field empty. Otherwise, we will only be able to run the Pod if it has access to the SCC nonroot-v2 or anyuid. Note that we cannot here define a specific userID because it will depend on the namespace. So, if we want to distribute an Operator using this image we do not know what is the ns where it will be installed and it begins to add complexities on top either if we are OK by running it with security context more permissve.

Therefore if we try a number like 1001 we will face issues like:

Leave the runAsUser field empty, or provide a value that falls within the specific user range for the namespace. provide a value in the ranges: [1000680000, 1000689999]

Then, you can also check its docs:

Lastly, the final USER declaration in the Dockerfile should specify the user ID (numeric value) and not the user name. This allows OpenShift Container Platform to validate the authority the image is attempting to run with and prevent running images

(and one is distinctly more user-friendly to read than the other). 😬

What do you mean with and one is distinctly more user-friendly to read than the other?
The UID and GID are defined in the Dockerfile already. (as described above)
It does not change its ux.

TL'DR: Regards the suggested solutions:

run the image explicitly as the user (or as any user -- it really doesn't matter which user you run this image as, and you could even choose a different one randomly every time it starts since there's no on-disk state which is what usually makes UID/GID changes difficult)

That is exactly what is not possible to do to achieve the goal.

maintain a short Dockerfile that's auto-built with just FROM memcached \n USER 11211:11211 (if it has to be baked into the image itself)

That is the alternative solution that I am trying to avoid with this PR. Note that I am checking it beforehand and others will also probably ask for the same in the future.

implement something like a mutating admission webhook on the cluster which can pull the image and resolve the UID/GID appropriately to then dynamically update the applied configuration appropriately (which I think is the most "Kubernetes" solution to this problem)

That does not address the problem.

I hope that this content and argumentations can help us to get this one merged :-)

@tianon @yosifkit @blar @ncopa @michael-k ( Could you please give a look at the reasons and re-consider accepting this small PR? )

@michael-k

This comment was marked as resolved.

@yosifkit
Copy link
Member

This is a duplicate of #36. We already define the user ID and its primary group ID; repeating the UID information later in the Dockerfile is not something we want to do as the username is much clearer to users than just a "random" ID (especially when doing docker inspect).

This seems like a failing of Kubernetes to not support looking up the defined username and of OpenShift in both not resolving the username to an ID and not having something like an alternate method of defining a non-root uid for the namespace mapping in "restricted-v2 SCC".

@camilamacedo86
Copy link
Author

camilamacedo86 commented Aug 23, 2022

HI @yosifkit,

This is a duplicate of #36. We already define the user ID and its primary group ID; repeating the UID information later in the Dockerfile is not something we want to do as the username is much clearer to users than just a "random" ID (especially when doing docker inspect).

The current approach does not follow the good practices defined in the Docker docs, please check all info shared here #80 to understand the motivations.

Screenshot 2022-08-23 at 20 27 41

This seems like a failing of Kubernetes to not support looking up the defined username and of OpenShift in both not resolving the username to an ID and not having something like an alternate method of defining a non-root uid for the namespace mapping in "restricted-v2 SCC".

That is not a failure.
That is how docker works.

@yosifkit
Copy link
Member

yosifkit commented Aug 23, 2022

RUN groupadd --system --gid 11211 memcache && useradd --system --gid memcache --uid 11211 memcache

We do set an explicit user ID when the user is created as the docs suggest (so it isn't non-deterministic) and we even set the primary group so that the following does not apply.

When the user doesn’t have a primary group then the image (or the next instructions) will be run with the root group.

So, no, I'd rather not repeat the value of that user ID in the USER directive just because the popular container orchestration platform does not try to look it up from the image.

@camilamacedo86
Copy link
Author

Closing since it was not accepted.

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

Successfully merging this pull request may close these issues.

4 participants