feat: changing owner while creating container for download support

[VietND96]

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

feat: changing owner while creating container for download support

Motivation and Context

Implement #1947 with highlight changes

• Create a script function fix-permissions with given list of directories needed to change ownership. This script is ensuring the ownership within seluser and Support Arbitrary User IDs (follows https://docs.openshift.com/container-platform/3.10/creating_images/guidelines.html#openshift-specific-guidelines).
• User info to run Selenium set as ARG, people can build with another user given.
• Via the entry point to handle changing ownership of the download directory is given via SE_DOWNLOAD_DIR. After changing, also have a step to verify directory can write, if not an ERROR is raised in stdout.
• ENV CHOWN_EXTRA allows the user to specify extra dirs that need to be fix-permissions. Multiple dirs are separated by comma ,. For example: -e CHOWN_EXTRA=/home/seluser/.vnc,/etc/certificates
• ENV MKDIR_EXTRA allows the user to specify extra dirs that need to be created with fix-permissions together. Multiple dirs are separated by comma ,
• Remove all chmod 777 somewhere.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[diemol]

A minor comment only.

I think this should not break support for platforms like OpenShift. Let's see what they say.

[diemol]

Thank you, @VietND96!

[alexblatov]

We are using your latest image as base in our project. Right after this change we start failing with:
Error: Command failed: docker exec -d greenkeeper-chromium-0 bash -c chromedriver --verbose --log-path=/home/seluser/chromium/chromedriver.log --whitelisted-ips 2>&1 | sudo tee /var/log/chromedriver.log Error response from daemon: Container 7004797cca42910a2c0b49de45f9f9950e846decc06fc3b297e6769207cecbd7 is not running

Any hints from your side to solve it @VietND96 ? Thanks!

[VietND96]

Hi @alexblatov, I saw the message that container is not running, can you share the container log during it was starting up to see any wrong happened there?
Also, are you using rootless in podman in this situation?

[alexblatov]

thanks for reply @VietND96 !
We can see this in container output:
Creating directory /home/seluser/Downloads Changing /home/seluser/Downloads ownership. /home/seluser/Downloads is owned by seluser ERROR: no write access to download dir /home/seluser/Downloads. Please correct the permissions and restart.

In out docker file we use:

ARG UID
ARG GID

# On linux, there are problems with file permissions
# See http://josephralph.co.uk/docker-linux-file-permission-handling/

# The seluser has UID/GID 1000/1000, which is very common on linux systems
# Build with --build-arg UID=$(id -u) --build-arg GID=$(id -g) to automatically get
# correct file permissions of bind mounts on Linux hosts. For Mac these args are not needed
RUN if [ ! -z $GID ]; then groupmod -g $GID seluser ; find / -ignore_readdir_race -group 1000 -exec chgrp -h seluser {} \; ; fi
RUN if [ ! -z $UID ]; then usermod -u $UID seluser ; find / -ignore_readdir_race -user 1000 -exec chown -h seluser {} \; ; fi

So we play with UID and GID a bit inside out code:

// If host is Linux - setting the uid/gid of docker user to match current user. Otherwise we can't share files and folders in bind mounts
        // See explanation https://github.com/docker/compose/issues/5507#issuecomment-353890002
        if (isLinux()) {
            buildArgs += " --build-arg UID=$(id -u) --build-arg GID=$(id -g)";
        }

Any thoughts?

[VietND96]

@alexblatov, I am also trying to fix something relates to that ERROR. I can see you are using user ids != 1200 and custom script to change ownership.
If it's easy to test from your end, can you take image tags in namespace ndviet/<image_name>:latest. If it works I will give a patch official soon.

[VietND96]

Hi @alexblatov, this feat is reverted in latest released tag '20231219'. Can you recheck the issue gets resolved?

[alexblatov]

Hi @alexblatov, this feat is reverted in latest released tag '20231219'. Can you recheck the issue gets resolved?

Having this now:
File "/usr/bin/supervisord", line 33, in <module> sys.exit(load_entry_point('supervisor==4.2.1', 'console_scripts', 'supervisord')()) File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 361, in main go(options) File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 371, in go d.main() File "/usr/lib/python3/dist-packages/supervisor/supervisord.py", line 72, in main self.options.make_logger() File "/usr/lib/python3/dist-packages/supervisor/options.py", line 1470, in make_logger loggers.handle_file( File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 417, in handle_file handler = RotatingFileHandler(filename, 'a', maxbytes, backups) File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 213, in __init__ FileHandler.__init__(self, filename, mode) File "/usr/lib/python3/dist-packages/supervisor/loggers.py", line 160, in __init__ self.stream = open(filename, mode) PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'

[VietND96]

Ohh, may I know the last image tag that it worked with your scenario?

[alexblatov]

Ohh, may I know the last image tag that it worked with your scenario?

latest tag on date 12 Dec 2023, worked. Latest tag from 13 Dec 2023 stopped working. @VietND96

[bobbyvacco]

adding password in environment variables violates security best practices and has caused palo alto(Prisma) vulnerability scans to fail. this type of feature may be best served by the recommendation to self-build the image since it's not a best practice.

[bobbyvacco]

adding password in environment variables violates security best practices and has caused palo alto(Prisma) vulnerability scans to fail. this type of feature may be best served by the recommendation to self-build the image since it's not a best practice.

another potential resolution for this that might work around scanners or be considered better practice:
set defaults for ARG rather than the ENV and always set ENV to ARG value. the default values should be clear "tokens" indicating they should be replaced such as "default_username" and "default_password"

[bobbyvacco]

nevermind. looks like this is solved here: #2061

[VietND96]

adding password in environment variables violates security best practices and has caused palo alto(Prisma) vulnerability scans to fail. this type of feature may be best served by the recommendation to self-build the image since it's not a best practice.

Thanks for your review, the password set default via ENV was fixed in this PR, no more ENV containing sensitive data. https://github.com/SeleniumHQ/docker-selenium/pull/2061/files
Can you review and feedback if any missing?

[VietND96]

Ohh, may I know the last image tag that it worked with your scenario?

latest tag on date 12 Dec 2023, worked. Latest tag from 13 Dec 2023 stopped working. @VietND96

PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'

@alexblatov, I guess before tag 20231212 it worked because of this

docker-selenium/Base/Dockerfile

Line 78 in 0b663b3

&& chmod -R 777 /opt/selenium /opt/selenium/assets /var/run/supervisor /var/log/supervisor /etc/passwd \

For now, after 20231212 it is

docker-selenium/Base/Dockerfile

Line 101 in af16a26

&& chmod -R 775 /opt/selenium /var/run/supervisor /var/log/supervisor /etc/passwd ${HOME} \

There was a concern on perm 777 very open, it needed to be reduced to 775 due to security reason (motivated in #1963)

---

As of now, I guess these use cases are fine to start the container:

1. docker run without --user. The default UID:GID 1200:1201 is used.
2. docker run --user $(id -u) .... The container start with arbitrary UIDs, which is handled properly for OpenShift support. In the CI, we also had tests random user to guarantee this works.

For your scenario, I saw probably you will start the container with the host UID:GID, which is docker run --user $(id -u):$(id -g) .... However, with the host UID:GID which is different from default 1200:1201 (others - users who are not the owner and not in the group).

Within chmod -R 775 /opt/selenium /var/run/supervisor /var/log/supervisor /etc/passwd ${HOME} \ . The others started supervisord don't have perm access write to /var/log/supervisor, and leads to PermissionError: [Errno 13] Permission denied: '/var/log/supervisor/supervisord.log'

I am also aware of this issue, in another PR after that #2064, I added back 777 for those dirs, but it is in another shell script, not put directly in Dockerfile to avoid security scan, all things pre-configured when building the image.

However, also the same that PR, there was a discussion around the point This feature is unnecessary, Permissions are not an issue if the container is started in the right way even without the chmod 777

After that, I went ahead to revert something, but still keep the change chmod 775 for /var/log/supervisor. PR #2069

At that time, I thought that, if user want to run the image with both host UID:GID, they have to rebuild the image, with root perm and --build-arg UID=$(id -u) --build-arg GID=$(id -g), the default 1200:1201 will be corrected to their host UID:GID. Since when starting the container with non-root user in entry point, we could not do any steps further to change the perm.

I am not sure that we can handle this without trace-off (container running well for both --user $(id -u):$(id -g) and no 777 perm given). Can you walk through all the changes above and see anything is wrong or can make it better? Or we must add back 777 for those dirs?

@diemol, can you also take a look and advise?

[diemol]

LGTM, let's wait for feedback.

[alexblatov]

thanks @VietND96 and @diemol ! All good from our side now. We used this hack to overcome 777 limits:
`RUN if [ ! -z $GID ]; then groupmod -g $GID seluser ; find / -ignore_readdir_race -group 1201 -exec chgrp -h seluser {} ; ; fi

RUN if [ ! -z $UID ]; then usermod -u $UID seluser ; find / -ignore_readdir_race -user 1200 -exec chown -h seluser {} ; ; fi`