search

docker-library / memcached

Docker Official Image packaging for memcached
http://memcached.org/
BSD 3-Clause "New" or "Revised" License
132 stars 99 forks source link

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

Closed camilamacedo86 closed 2 years ago

camilamacedo86 commented 2 years ago

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:

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

camilamacedo86 commented 2 years ago

@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 commented 2 years ago

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). :grimacing:

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

camilamacedo86 commented 2 years ago

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 commented 2 years ago

Please stop mentioning me. I'm not a maintainer of this repository and I don't work for Docker either.

yosifkit commented 2 years ago

This is a duplicate of https://github.com/docker-library/memcached/issues/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 commented 2 years ago

HI @yosifkit,

This is a duplicate of https://github.com/docker-library/memcached/issues/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 https://github.com/docker-library/memcached/issues/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 commented 2 years ago

https://github.com/docker-library/memcached/blob/1f2afc73fbae68b30d0d0455decf60b1675af513/debian/Dockerfile#L4

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 commented 2 years ago

Closing since it was not accepted.