From 56fc22aca7c7552e0f9d56a59b48fb0c55fb1e9f Mon Sep 17 00:00:00 2001 From: Viet Nguyen Duc Date: Fri, 15 Dec 2023 16:49:24 +0530 Subject: [PATCH] fix: changing owner while creating container for download support (#2064) Signed-off-by: Viet Nguyen Duc --- .github/workflows/helm-chart-test.yml | 4 +- Base/Dockerfile | 4 +- Base/entry_point.sh | 60 ++++++++++--------------- Base/fix-permissions | 65 ++++++++++++++++++--------- NodeChrome/Dockerfile | 1 - NodeEdge/Dockerfile | 1 - README.md | 56 ++++++++++++++++------- tests/test.py | 2 +- 8 files changed, 113 insertions(+), 80 deletions(-) diff --git a/.github/workflows/helm-chart-test.yml b/.github/workflows/helm-chart-test.yml index b81788951..887e6b5da 100644 --- a/.github/workflows/helm-chart-test.yml +++ b/.github/workflows/helm-chart-test.yml @@ -65,12 +65,12 @@ jobs: if: always() uses: actions/upload-artifact@v4 with: - name: ${{ env.CHART_FILE_NAME }} + name: ${{ matrix.test-strategy }}_${{ env.CHART_FILE_NAME }} path: ${{ env.CHART_PACKAGE_PATH }} - name: Upload Helm chart template rendered if: always() uses: actions/upload-artifact@v4 with: - name: chart_template_rendered.yaml + name: ${{ matrix.test-strategy }}_chart_template_rendered.yaml path: ./tests/tests/output_deployment.yaml if-no-files-found: ignore diff --git a/Base/Dockerfile b/Base/Dockerfile index 6bda097b7..ec37f8110 100644 --- a/Base/Dockerfile +++ b/Base/Dockerfile @@ -133,8 +133,7 @@ RUN /tmp/cs fetch --classpath --cache ${EXTERNAL_JARS} \ RUN rm -fr /root/.cache/* # Change ownership of directories -RUN chown -R "${SEL_USER}:${SEL_GID}" ${HOME} ${SEL_DIR} ${SEL_DIR}/assets ${EXTERNAL_JARS} ${SE_DOWNLOAD_DIR} /var/run/supervisor /var/log/supervisor \ - && fix-permissions ${HOME} ${SEL_DIR} ${SEL_DIR}/assets ${EXTERNAL_JARS} ${SE_DOWNLOAD_DIR} /var/run/supervisor /var/log/supervisor +RUN fix-permissions ${HOME} ${SEL_DIR} ${SEL_DIR}/assets ${EXTERNAL_JARS} ${SE_DOWNLOAD_DIR} /var/run/supervisor /var/log/supervisor #========== # Relaxing permissions for OpenShift and other non-sudo environments @@ -145,6 +144,7 @@ RUN chmod g=u /etc/passwd # Run the following commands as non-privileged user #=================================================== USER ${SEL_UID}:${SEL_GID} +VOLUME ${SE_DOWNLOAD_DIR} # Boolean value, maps "--bind-host" ENV SE_BIND_HOST false diff --git a/Base/entry_point.sh b/Base/entry_point.sh index 4ef9ee26b..0e8ef12f3 100755 --- a/Base/entry_point.sh +++ b/Base/entry_point.sh @@ -5,44 +5,30 @@ _log () { fi } -#============================================== -# OpenShift or non-sudo environments support -# https://docs.openshift.com/container-platform/3.11/creating_images/guidelines.html#openshift-specific-guidelines -#============================================== - -if ! whoami &> /dev/null; then - if [ -w /etc/passwd ]; then - echo "${USER_NAME:-${SEL_USER}}:x:$(id -u):0:${USER_NAME:-${SEL_USER}} user:${HOME}:${SE_DOWNLOAD_DIR}:/var:/opt:/sbin/nologin" >> /etc/passwd - fi -fi - -MKDIR_EXTRA=${SE_DOWNLOAD_DIR}","${MKDIR_EXTRA} -CHOWN_EXTRA=${MKDIR_EXTRA}","${CHOWN_EXTRA} - -if [ -n "${MKDIR_EXTRA}" ]; then - for extra_dir in $(echo "${MKDIR_EXTRA}" | tr ',' ' '); do - _log "Creating directory ${extra_dir} ${MKDIR_EXTRA_OPTS:+(mkdir options: ${MKDIR_EXTRA_OPTS})}" - # shellcheck disable=SC2086 - sudo mkdir ${MKDIR_EXTRA_OPTS:-"-p"} "${extra_dir}" - done -fi - -if [ -n "${CHOWN_EXTRA}" ]; then - for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do - _log "Changing ${extra_dir} ownership. ${extra_dir} is owned by ${SEL_USER} ${CHOWN_EXTRA_OPTS:+(chown options: ${CHOWN_EXTRA_OPTS})}" - # shellcheck disable=SC2086 - sudo chown ${CHOWN_EXTRA_OPTS:-"-R"} "${SEL_UID}:${SEL_GID}" "${extra_dir}" - sudo -E fix-permissions "${extra_dir}" - done -fi - -# Raise error if the user isn't able to write files to download dir -if [ -n "${CHOWN_EXTRA}" ]; then - for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do - if [[ ! -w ${extra_dir} ]]; then - _log "ERROR: no write access to download dir ${SE_DOWNLOAD_DIR}. Please correct the permissions and restart." +# If the container started as the root user +if [ "$(id -u)" == 0 ]; then + fix-permissions "${SE_DOWNLOAD_DIR}" +elif [ "$(id -u)" == "$(id -u ${SEL_USER})" ] && [ "$(id -g)" == "$(id -g ${SEL_USER})" ]; then + # Trust SEL_USER is the desired non-root user to execute with sudo + sudo -E fix-permissions "${SE_DOWNLOAD_DIR}" +else + # For non-root user to change ownership + # Relaxing permissions for OpenShift and other non-sudo environments + # (https://docs.openshift.com/container-platform/latest/openshift_images/create-images.html#use-uid_create-images) + if ! whoami &> /dev/null; then + _log "There is no entry in /etc/passwd for our UID=$(id -u). Attempting to fix..." + if [ -w /etc/passwd ]; then + _log "Renaming user to ${USER_NAME:-default} ($(id -u):$(id -g)" + # We cannot use "sed --in-place" since sed tries to create a temp file in + # /etc/ and we may not have write access. Apply sed on our own temp file: + sed --expression="s/^${SEL_USER}:/${USER_NAME:-default}:/" /etc/passwd > /tmp/passwd + echo "${USER_NAME:-default}:x:$(id -u):$(id -g):${USER_NAME:-default} user:${HOME}:/bin/bash" >> /tmp/passwd + cat /tmp/passwd > /etc/passwd + rm /tmp/passwd + _log "Added new ${USER_NAME:-default} user ($(id -u):$(id -g)). Fixed UID!" fi - done + fi + fix-permissions "${SE_DOWNLOAD_DIR}" fi /usr/bin/supervisord --configuration /etc/supervisord.conf & diff --git a/Base/fix-permissions b/Base/fix-permissions index 97d4ad8b8..e20ab2dbc 100644 --- a/Base/fix-permissions +++ b/Base/fix-permissions @@ -1,23 +1,46 @@ #!/bin/bash set -e -# Run this with USER root only -for d in "$@"; do - find "${d}" \ - ! \( \ - -group "${SEL_GID}" \ - -a -perm -g+rwX \ - \) \ - -exec chgrp -R "${SEL_GID}" -- {} \+ \ - -exec chmod -R g+rwX -- {} \+ - # setuid, setgid *on directories only* - find "${d}" \ - \( \ - -type d \ - -a ! -perm -6000 \ - \) \ - -exec chmod -R +6000 -- {} \+ - # Relaxing permissions for OpenShift and other non-sudo environments - chmod -R u+x "${d}" - chgrp -R 0 "${d}" - chmod -R g=u "${d}" -done + +if [[ "${SE_DOWNLOAD_DIR}" != "${HOME}/Downloads" ]] && [[ -d "${SE_DOWNLOAD_DIR}" ]]; then + mkdir -p "${SE_DOWNLOAD_DIR}" +fi + +if [ "$(id -u)" == 0 ]; then + # For root user to change ownership + for d in "$@"; do + find "${d}" \ + ! \( \ + -group "${SEL_GID}" \ + -a -perm -g+rwX \ + \) \ + -exec chown -R "${SEL_UID}:${SEL_GID}" -- {} \+ \ + -exec chgrp -R "${SEL_GID}" -- {} \+ \ + -exec chmod -R g+rwX -- {} \+ + find "${d}" \ + \( \ + -type d \ + -a ! -perm -775 \ + \) \ + -exec chmod -R 775 -- {} \+ + # Relaxing permissions for OpenShift and other non-sudo environments + chmod -R u+x "${d}" + chgrp -R 0 "${d}" + chmod -R g=u "${d}" + done + + # Only give write access for app data in case running of read-only filesystem + dirs=( + "${HOME}" + "${SEL_DIR}" + "${SE_DOWNLOAD_DIR}" + "/var/run/supervisor" + "/var/log/supervisor" + "/etc/passwd" + "/tmp/.X11-unix" + ) + for d in "${dirs[@]}"; do + if [[ -d $d ]]; then + chmod 777 -R "${d}" + fi + done +fi diff --git a/NodeChrome/Dockerfile b/NodeChrome/Dockerfile index e52dd5798..aee098943 100644 --- a/NodeChrome/Dockerfile +++ b/NodeChrome/Dockerfile @@ -50,7 +50,6 @@ RUN if [ ! -z "$CHROME_DRIVER_VERSION" ]; \ && unzip /tmp/chromedriver_linux64.zip -d /opt/selenium \ && rm /tmp/chromedriver_linux64.zip \ && mv /opt/selenium/chromedriver-linux64/chromedriver /opt/selenium/chromedriver-$CHROME_DRIVER_VERSION \ - && fix-permissions /opt/selenium/chromedriver-$CHROME_DRIVER_VERSION \ && ln -fs /opt/selenium/chromedriver-$CHROME_DRIVER_VERSION /usr/bin/chromedriver USER ${SEL_UID} diff --git a/NodeEdge/Dockerfile b/NodeEdge/Dockerfile index 2e3c781e8..500862f0f 100644 --- a/NodeEdge/Dockerfile +++ b/NodeEdge/Dockerfile @@ -43,7 +43,6 @@ RUN if [ -z "$EDGE_DRIVER_VERSION" ]; \ && unzip /tmp/msedgedriver_linux64.zip -d /opt/selenium \ && rm /tmp/msedgedriver_linux64.zip \ && mv /opt/selenium/msedgedriver /opt/selenium/msedgedriver-$EDGE_DRIVER_VERSION \ - && fix-permissions /opt/selenium/msedgedriver-$EDGE_DRIVER_VERSION \ && ln -fs /opt/selenium/msedgedriver-$EDGE_DRIVER_VERSION /usr/bin/msedgedriver USER ${SEL_UID} diff --git a/README.md b/README.md index 56287ac3a..ec9e6b535 100644 --- a/README.md +++ b/README.md @@ -1302,46 +1302,72 @@ that directory because it is running under the user `seluser`. This happens because that is how Docker mounts volumes in Linux, more details in this [issue](https://github.com/moby/moby/issues/2259). -There was a fix in this feature [#1947](https://github.com/SeleniumHQ/docker-selenium/issues/1947) -that changed ownership when staring the container. +A workaround (to be done manually) for this is to create a directory on the +host and change its permissions **before mounting the volume**. +Depending on your user permissions, you might need to use +`sudo` for some of these commands: -You are able to configure browser with another download directory and mount the host with it in container by overriding `SE_DOWNLOAD_DIR`. +```bash +mkdir /home/ubuntu/files +chown 1200:1201 /home/ubuntu/files +``` + +After doing this, you should be able to download files +to the mounted directory. + +--- +Another introduced feature [#1947](https://github.com/SeleniumHQ/docker-selenium/issues/1947) +that take action to change ownership when staring the container. +You are able to configure another default browser download directory and mount the host with it in container by overriding `SE_DOWNLOAD_DIR`. + +For example, in test you might be scripting something ```groovy ChromeOptions options = new ChromeOptions(); HashMap chromePrefs = new HashMap(); -chromePrefs.put("download.default_directory", "/tmp/downloads"); +chromePrefs.put("download.default_directory", "/path/to/your/downloads"); options.setExperimentalOption("prefs", chromePrefs); options.add_argument('disable-features=DownloadBubble,DownloadBubbleV2') WebDriver driver = new ChromeDriver(options); ``` +When running the container, you set the `SE_DOWNLOAD_DIR` and mount the host with that directory in container. ```bash docker run -d -p 4444:4444 --shm-size="2g" \ - -e SE_DOWNLOAD_DIR=/tmp/downloads \ - -v /home/ubuntu/files:/tmp/downloads \ + -e SE_DOWNLOAD_DIR=/path/to/your/downloads \ + -v /home/ubuntu/files:/path/to/your/downloads \ selenium/standalone-chrome:4.16.1-20231212 ``` -### Change ownership of the volume mount +**Note:** The changing ownership when starting container is not supported well when both overriding `SE_DOWNLOAD_DIR` and running non-root (e.g. Podman) or specifying user ids different from `1200` (OpenShift arbitrary user ids). + +In this case, you can use above workaround to create and set permissions for the directory on the host before mounting the volume. -If you are using Linux and you need to change the ownership of the volume mount, you can set the `CHOWN_EXTRA` and `CHOWN_EXTRA_OPTS` (default is set `-R` - change recursively) environment variables +You also can run the container with `--user root` once to initialize and change ownership of the directory on the host, then run the container with non-root user again. +For example, the first run with root user: ```bash docker run -d -p 4444:4444 --shm-size="2g" \ - -v /home/ubuntu/my-certs:/etc/certs \ - -e CHOWN_EXTRA=/etc/certs \ + --user root \ + -e SE_DOWNLOAD_DIR=/path/to/your/downloads \ + -v /home/ubuntu/files:/path/to/your/downloads \ selenium/standalone-chrome:4.16.1-20231212 ``` -If you want a new volume mount directory to be created and set ownership, you can set the `MKDIR_EXTRA` and `MKDIR_EXTRA_OPTS` (default is set `-p` - create a directory hierarchy) environment variables. - +Then stop it, rerun with switching to non-root user: ```bash docker run -d -p 4444:4444 --shm-size="2g" \ - -v /home/ubuntu/my-nssdb:/home/seluser/.pki/nssdb \ - -e MKDIR_EXTRA=/home/seluser/.pki/nssdb \ + --user 4496 \ + -e SE_DOWNLOAD_DIR=/path/to/your/downloads \ + -v /home/ubuntu/files:/path/to/your/downloads \ selenium/standalone-chrome:4.16.1-20231212 ``` +Summarize the supported use case for changing ownership when starting container: -Both `CHOWN_EXTRA` and `MKDIR_EXTRA` can be set to multiple directories by separating them with a `space` or `comma`. For example: `CHOWN_EXTRA=,` +| User (uid) | Mount `SE_DOWNLOAD_DIR` in container | Auto changing | +|----------------------|--------------------------------------|---------------| +| seluser (uid `1200`) | default `/home/seluser/Downloads` | Yes | +| seluser (uid `1200`) | any `/path/to/downloads` | Yes | +| any (uid != `1200`) | default `/home/seluser/Downloads` | Yes | +| any (uid != `1200`) | any `/path/to/downloads` | No | diff --git a/tests/test.py b/tests/test.py index b8b462c2b..0d260e330 100644 --- a/tests/test.py +++ b/tests/test.py @@ -174,7 +174,7 @@ def standalone_browser_container_matches(container): use_random_user_id = USE_RANDOM_USER_ID == 'true' run_in_docker_compose = RUN_IN_DOCKER_COMPOSE == 'true' - random_user_id = random.randint(2000, 65000) + random_user_id = "%s:%s" % (random.randint(2000, 65000), random.randint(2001, 65001)) if use_random_user_id: logger.info("Running tests with a random user ID -> %s" % random_user_id)