thecodingmachine / docker-images-nodejs

A set of Node.js Docker images
MIT License
10 stars 8 forks source link

sudo fails to run in k8s #14

Open telmich opened 2 years ago

telmich commented 2 years ago

When trying to run thecodingmachine/workadventure-front:v1.6.4 it fails with:

[10:58] nb3:generic% kubectl logs -f front-5f7d75f9cb-9n6xc 
sudo: unable to send audit message
sudo: pam_open_session: System error
sudo: policy plugin failed session initialization

Changing the cmd to shell and testing manually:

docker@front-758dbc949-jm2v2:/var/www/html$ sudo -i
sudo: unable to send audit message
sudo: pam_open_session: System error
sudo: policy plugin failed session initialization

Seeing that the initial script switches directly to execute sudo tini, I wonder if it wouldn't make sense to...

mistraloz commented 2 years ago

Hi @telmich, i'm agree about the UID 0 but if we do that, it's will generate a breaking change if any dependent image have a Dockerfile ended by USER docker (the recommanded good pratice... even if it's has no sense with sudo allowed). I will look for a solution but tell me if you have any more informations/suggests.

telmich commented 2 years ago

Good morning @mistraloz! I think the USER docker makes sense in docker environments, not so much though in k8s environments, TBH.

I have seen that your back/front containers are separated, but they are derived from the same base image. If you want to avoid breaking existing users, I'd suggest the following:

If you happen to have a chat (slack, matrix, etc.) I can also join there, as I would be open to some improvements, maybe even supporting an -alpine version.

mistraloz commented 2 years ago

1/ If you know how to create one without required of sudo usage, do it, it's the best way :). 2/ Otherwise instead of -nosudo / -root suffix, we may have an environment to choice (or just test the current user, if it's root, with don't need sudo). 3/ An alpine version may be very nice but it's another point. If you would like to create one, we can have both (alpine and debian) but please create another issue for that, I will follow this PR with you. I'm agree for a tag "-alpine" (or "-slim"). 4/ I think this issue it's provide by a miss configuration in sudoers config. So we can fix it too.

I'm not available for 3hours but after i may discuss with you by chat if you are available.

moufmouf commented 2 years ago

In fact, in Kubernetes, by default, you cannot use "sudo" in the containers unless you explicitly allow privilege escalation (which is documented here: https://github.com/thecodingmachine/docker-images-nodejs#usage-in-kubernetes )

Indeed, we do this for Docker environments, so that the running user in Docker is the same as the current user of the OS (which is helpful if your process is writing files). We inherited this behavior from the TheCodingmachine Docker PHP image (https://github.com/thecodingmachine/docker-images-php) where this is a must have (PHP processes usually write their cache in files and Docker containers often have write issues because the users of Docker images do not match with the users of the host).

I'm not 100% sure making a variant of the image is the way to go. Because if you build images on top of those images, you will have to make several variants.

The best possible solution would be to be able to detect (somehow, I don't really know how), that we are running in Kubernetes and to avoid using "sudo" in this case. If we can detect that the "/usr/src/app" directory is NOT mounted (but part of the image itself), then there is no point in changing the user ID dynamically as we do, and therefore, no point in running sudo ?

telmich commented 2 years ago

I prefer staying slim/keeping the number of variants down, @moufmouf, but due to the way how the image is built, by default the userid is set to docker at the moment.

We did patch our way around this by using

securityContext:
    runAsUser: 0

(compare https://code.ungleich.ch/ungleich-public/ungleich-k8s/src/branch/master/apps/workadventure/v3/templates/back-deployment.yaml)

So on entrance point, the user is already set to docker by default and if the image is not changed, you cannot easily revert this.

In other words: to be k8s native, the image will need to not switch away user in the first place, which is currently part of the Dockerfile.

Does it make sense?

telmich commented 2 years ago

Comparing with https://github.com/thecodingmachine/docker-images-nodejs/blob/master/Dockerfile.14-apache:

RUN useradd -ms /bin/bash docker && adduser docker sudo
 # Users in the sudoers group can sudo as root without password.
 RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers
...
USER docker
...
RUN mkdir -p /var/www/html && chown docker:docker /var/www/html
...
ENV APACHE_RUN_USER=docker \
    APACHE_RUN_GROUP=docker

could be removed and additionally the entrypoint scripts could either be modified or a new one that is made for running without sudo could be added.

It might be possible to create the image like this and use docker-compose with the user: attribute to force another user, if necessary.

mistraloz commented 2 years ago

I'm not sure but this part may not require any changes. I think the mandatory changes are here This file is used to :

So to make a PR who accept to be run without sudoers, I think we have to :

telmich commented 2 years ago

The item "Find a solution to run a2enmod as docker user" can be easily solved by

chown docker /etc/apache2/mods-enabled/

in the Dockerfile.

In regards to autodetection vs. SUDO_DISABLED=true, I would have a strong preference on the latter, as it is explicit and might be used in other scenarios as well.

mistraloz commented 2 years ago

I'm suspicious of solutions that seem too easy :p ... but you have right, it's seem to be the only need. Can you make a PR for that ? I'm will be on holidays and make changes during the christmas sales may be not a good idea but i can merge that early january.

ericzolf commented 2 years ago

I don't know how far it is related but I was able to reproduce the same sudo issue with debian:stretch-slim, on which this image is based, and fixing it by using debian:bullseye-slim instead. Could it be that upgrading the base for the image would fix the issue? see https://github.com/thecodingmachine/workadventure/issues/1925