ci: run docker wrapper with a non-root user

josibake commented at 12:50 pm on August 22, 2022: member

Previously, everything in the ci docker image ran as the root user. This would lead to certain directories (ci/scratch, depends) being owned by root after running the ci locally which would lead to annoying behavior such as subsequent guix builds failing due to depends/ being owned by root.

This PR adds a non-root user in the container and chowns the mounted working directory. All the docker exec commands now run as the non-root user, except for the few that still need to run as root (mainly, installing packages).

To test this I checked out a fresh copy of the repo, applied my changes, ran the CI, and verified all the local file permissions were unchanged after the CI was finished running.

fanquake added the label Tests on Aug 22, 2022

hebasto commented at 2:25 pm on August 22, 2022: member

Concept ACK.

Fix linter failures?

in ci/entrypoint.sh:13 in 62cc872a76 outdated

 8+echo "Starting with UID: $USER_ID, GID: $GROUP_ID"
 9+useradd -u $USER_ID -o -m $USER_NAME
10+groupmod -g $GROUP_ID $USER_NAME
11+
12+chown -R $USER_NAME:$USER_NAME ${WORKING_DIR}
13+tail -f /dev/null

maflcko commented at 2:37 pm on August 22, 2022:

what is this line doing and why?

josibake commented at 2:53 pm on August 22, 2022:

Without this line, docker run .. would hit the entrypoint.sh script and then exit once the script was finished. Adding this line keeps the container running without using any CPU inside the container so that we can then run all of the docker exec commands against the running container

maflcko commented at 2:55 pm on August 22, 2022:

Why not sleep inf?

josibake commented at 3:11 pm on August 22, 2022:

I don’t have a strong opinion about one over the other. I’ve almost always seen tail -f /dev/null as the recommended solution for docker and IIRC sleep inf had some issues with portability back in the day

in ci/test/04_install.sh:36 in 62cc872a76 outdated

32@@ -33,7 +33,7 @@ export P_CI_DIR="$PWD"
33 
34 if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
35   echo "Creating $DOCKER_NAME_TAG container to run in"
36-  ${CI_RETRY_EXE} docker pull "$DOCKER_NAME_TAG"
37+  docker build ${BASE_ROOT_DIR}/ci -t "$DOCKER_NAME_TAG" --build-arg DOCKER_NAME_TAG=$DOCKER_NAME_TAG

maflcko commented at 2:37 pm on August 22, 2022:

I am not familiar with docker, does this overwrite the system image for, let’s say, ubuntu:focal?

josibake commented at 2:54 pm on August 22, 2022:

ah, that’s a good point, I should probably add something extra to the tag to make sure there are no weird conflicts later with your local images

maflcko commented at 3:04 pm on August 22, 2022:

Maybe just keep everything as it was and only run useradd ... (and the other stuff from entry.sh) after docker run?

josibake commented at 3:37 pm on August 22, 2022:

hm, I’d prefer to do it through the entrypoint script as it’s more clear (to me, anyway) as to what’s going on. It also keeps all the docker permissions stuff together, which feels better than mixing more logic for setting up docker into the install script

maflcko commented at 3:44 pm on August 22, 2022:

What about manually calling the entry script? The reason I suggest this is to avoid the potential interaction with system images, or the need for sleep inf, or the need for a dockerfile

in ci/test/04_install.sh:73 in 62cc872a76 outdated

63 }
64+CI_EXEC_ROOT () {
65+  $DOCKER_CI_CMD_PREFIX_ROOT bash -c "export PATH=$BASE_SCRATCH_DIR/bins/:\$PATH && cd \"$P_CI_DIR\" && $*"
66+}
67 export -f CI_EXEC
68+export -f CI_EXEC_ROOT

maflcko commented at 2:39 pm on August 22, 2022:

what about removing this and using sudo? I guess sudo isn’t installed?

josibake commented at 2:59 pm on August 22, 2022:

sudo isn’t installed in the container and it’s generally not recommended to install or use it inside docker. I renamed the variable here to make it explicit, but this is the same behavior as before: if we don’t specify a user (docker exec -u <some user>), docker defaults to running root inside the container. The thing that has changed is now when we run CI_EXEC, we are running as our newly created non-root user.

w0xlt approved

w0xlt commented at 3:09 pm on August 22, 2022: contributor

Concept ACK

DrahtBot commented at 9:30 am on September 23, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto
Concept ACK	w0xlt
Stale ACK	satsie

Conflicts

No conflicts as of last run.

satsie commented at 8:26 pm on September 27, 2022: contributor

Concept ACK. I hope to circle back and do a code review.

I tested this PR, and things behave as expected. In addition to doing a spot check on the directories, I made an attempt to tally up all the files that had ownership changes after running the CI (./ci/test_run_all.sh).

Number of files that had some kind of change to their read/write/attribute/execute permissions:

master branch: 25,806
this PR: 41

For both scenarios I made a fresh directory and clone of the code base. I used auditctl to monitor the files and documented my steps here: https://gist.github.com/satsie/a73fe711954dc4bca8b43030bbd4fb2d

I’m completely new to auditctl so I wouldn’t be surprised if there are improvements that can be made to my process, but my takeaway is this PR results in far fewer changes in file ownership (yay!).

For the 41 file permission changes that were present with this PR, I see that a lot of them are the result of git commands, and mkdir, so it’s very likely this PR got them all. Happy to iterate on this if anyone has more insight, especially regarding how to use auditctl.

maflcko added the label Waiting for author on Sep 28, 2022

DrahtBot added the label Needs rebase on Oct 4, 2022

josibake force-pushed on Oct 6, 2022

josibake commented at 8:32 pm on October 6, 2022: member

I reworked this to incorporate feedback from @MarcoFalke: removed the Dockerfile and entrypoint.sh script and now create the non-root user directly in the running docker container. Also fixed the linting errors. @satsie thanks for providing the testing script! I ran it and confirmed local file permissions were unchanged after running the ci script.

DrahtBot removed the label Needs rebase on Oct 6, 2022

maflcko removed the label Waiting for author on Oct 7, 2022

in ci/test/05_before_script.sh:14 in 190b4f3d53 outdated

 9@@ -10,7 +10,7 @@ export LC_ALL=C.UTF-8
10 if [ "$CI_OS_NAME" == "macos" ]; then
11   echo > "${HOME}/Library/Application Support/Bitcoin"
12 else
13-  CI_EXEC echo \> \$HOME/.bitcoin
14+  CI_EXEC_ROOT echo \> \$HOME/.bitcoin

maflcko commented at 2:36 pm on October 7, 2022:

This looks wrong? Maybe just create both to be save?

josibake commented at 3:18 pm on October 7, 2022:

not sure why i originally put this here :thinking: i removed it and everything still worked

maflcko commented at 3:39 pm on October 7, 2022:

The idea is that the main directory should not be read/created when running the tests. This is achieved by creating a file with that name. Now that there are two users, it seems safer to create two files.

in ci/test/04_install.sh:102 in 190b4f3d53 outdated

 98@@ -81,7 +99,7 @@ if [ -n "$PIP_PACKAGES" ]; then
 99     IN_GETOPT_BIN="/usr/local/opt/gnu-getopt/bin/getopt" ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES
100   else
101     # shellcheck disable=SC2086
102-    ${CI_RETRY_EXE} CI_EXEC pip3 install --user $PIP_PACKAGES
103+    ${CI_RETRY_EXE} CI_EXEC_ROOT pip3 install --user $PIP_PACKAGES

maflcko commented at 2:36 pm on October 7, 2022:

Is this needed?

josibake commented at 3:19 pm on October 7, 2022:

good catch, removed it and pip3 was still able to install.

josibake force-pushed on Oct 7, 2022

josibake commented at 3:21 pm on October 7, 2022: member

removed to extraneous _ROOT calls; git range-diff master 190b4f3 05ce35e

hebasto approved

hebasto commented at 2:00 pm on October 10, 2022: member

ACK 05ce35efaa77f4dfe941361cf2c41642cf150449, I have reviewed the code and it looks OK. Also tested on Ubuntu 22.04: no root-owned files were detected after execution of:

0$ make distclean
1$ env MAKEJOBS="-j16" FILE_ENV="./ci/test/00_setup_env_win64.sh" ./ci/test_run_all.sh

hebasto commented at 2:28 pm on October 10, 2022: member

After more tests done, suggesting:

 0--- a/ci/test/04_install.sh
 1+++ b/ci/test/04_install.sh
 2@@ -144,7 +144,7 @@ if [[ "${RUN_TIDY}" == "true" ]]; then
 3     CI_EXEC "mkdir -p ${DIR_IWYU}/build/"
 4     CI_EXEC "git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_14 ${DIR_IWYU}/include-what-you-use"
 5     CI_EXEC "cd ${DIR_IWYU}/build && cmake -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-14 ../include-what-you-use"
 6-    CI_EXEC "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
 7+    CI_EXEC_ROOT "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
 8   fi
 9 fi
10

satsie commented at 2:20 am on October 11, 2022: contributor

ACK 05ce35efaa77f4dfe941361cf2c41642cf150449 - I tested the latest changes by performing the following steps on (1.) the master branch, and (2.) a fresh clone of the repo with this PR. I’m using Ubuntu 22.04, with the new 6.0 Linux kernel:

./autogen.sh
./configure.sh
make
./ci/test_run_all.sh

Per hebasto’s comment, realized there is a much, much easier way to check that this PR is working :sweat_smile: Instead of the auditctl commands I mentioned earlier, I simply checked the directory to make sure root didn’t own any files.

Master branch:

0find bitcoin/ -user root | wc -l
117315

This PR:

0find bitcoin/ -user root | wc -l
10

Since you mentioned failing guix builds as a motivator for this PR, and I need the practice, I went ahead and ran a guix build:

 0guix.sigs/05ce35efaa77$ cat satsie/noncodesigned.SHA256SUMS
 1510ba7aa76869af85b586be1f9aa9110cf454abdc7047092f6e8765ad024df69  bitcoin-05ce35efaa77-aarch64-linux-gnu-debug.tar.gz
 2d0f07868e1cf7fc440c3ff551cf651f7ab8b3badbb28fc1f514896c74cf13384  bitcoin-05ce35efaa77-aarch64-linux-gnu.tar.gz
 320bcc58ef04c2bde2573ebfe7265c910862f550517d5d2436e92c80da99090cd  bitcoin-05ce35efaa77-arm-linux-gnueabihf-debug.tar.gz
 4be95018d0be0a8578f31a72517725bbd3026f3c533a852eb367de7485bf0d3a6  bitcoin-05ce35efaa77-arm-linux-gnueabihf.tar.gz
 55d3b941b1121355df626c58d752ebf995c5cb4894ca94aea01ac9c7da3460d6f  bitcoin-05ce35efaa77-arm64-apple-darwin-unsigned.dmg
 6a9364fddf9a0af1dec52fcad43d786a4b6b198e2ce3dcda3b7e48e31e1e96190  bitcoin-05ce35efaa77-arm64-apple-darwin-unsigned.tar.gz
 7a1545413dec498fe503da178312e10790f8ce6324be677fd13a00c7749a360d5  bitcoin-05ce35efaa77-arm64-apple-darwin.tar.gz
 8afa502db06c93bbfee44cd193f59be614b91181590225c5d4b4ec5a84dfdf0a8  bitcoin-05ce35efaa77.tar.gz
 98268f616fb0a8bf04da556331e92ccbdbc6f627cd078f923ca9629184995586f  bitcoin-05ce35efaa77-powerpc64-linux-gnu-debug.tar.gz
10a2fde1f7d89702dc950e830ef34664caa9790944b62cf8cc09bb3a93b2588f25  bitcoin-05ce35efaa77-powerpc64-linux-gnu.tar.gz
1155b8a6415a99e33389120552e12dd98e0d293bbcb28cddcfcfd49cf9773425c1  bitcoin-05ce35efaa77-powerpc64le-linux-gnu-debug.tar.gz
12cedfe14cec343f9895f090446b84075aaa9bacc824aa380b89e225382868cee0  bitcoin-05ce35efaa77-powerpc64le-linux-gnu.tar.gz
13bf2961331c1161b9701d7d79462705d05dbdf1f5c29c1f2a1b1c6391de3aa46c  bitcoin-05ce35efaa77-riscv64-linux-gnu-debug.tar.gz
14c3451bfb427ac6e346f4fecff1e76b9f08702d877ed19923e1dff706f65cfd32  bitcoin-05ce35efaa77-riscv64-linux-gnu.tar.gz
15c0c0d8638b22a75c3e0efd7f31da1c5bed208ef6bf65544ef15ebce309896f28  bitcoin-05ce35efaa77-x86_64-apple-darwin-unsigned.dmg
16bb7920fbfe3ac98b1cc17cf06771e5c5601d8c896caf95c3e5c63ccaa054bc52  bitcoin-05ce35efaa77-x86_64-apple-darwin-unsigned.tar.gz
17e8b6bad3ca0252aa65e86e32b38f44cb7bff2cb82da7b7ecade8f727b3405c32  bitcoin-05ce35efaa77-x86_64-apple-darwin.tar.gz
18af4bb718b88c143dc9873e9be16d2c1dda286e5bceae9607ba6dd6755ed4cd5c  bitcoin-05ce35efaa77-x86_64-linux-gnu-debug.tar.gz
193df38517b0162fe21f2fbfba3c3d7b6eac799285f409dd5bbe84deb78efbac84  bitcoin-05ce35efaa77-x86_64-linux-gnu.tar.gz
20d75f46751f4562fa4316009dd5a7fed7418e686c11392064b86e0b229cac4fee  bitcoin-05ce35efaa77-win64-debug.zip
2170acfd1c279a65a51992756801e0d9a138c8fa1f155d795fb19a8686bf5d3a16  bitcoin-05ce35efaa77-win64-setup-unsigned.exe
22e6e27ff5b20d41659b5b58c6735c7e89fecbb947ad4d0640eb8aa1f7bddf0d62  bitcoin-05ce35efaa77-win64-unsigned.tar.gz
2354f4dfbedd651b837e20f39b205670e55fe29ddae9cf867001ce078e69a46730  bitcoin-05ce35efaa77-win64.zip

As far as code review goes, I appreciate the effort you put in to get all the commands into the 04_install.sh script. I didn’t get a chance to fully review the changes prior, but this new changeset is very straightforward and easy to read through. The addition of the new non root user that maps to the current one (in both ID and group ID), and the careful execution of only certain commands as root looks good to me.

maflcko commented at 6:48 am on October 11, 2022: member

This is still open: #25900 (review)

hebasto commented at 2:23 pm on November 11, 2022: member

@josibake

It seems the only changes, which are required to get this nice PR merged, are as follows:

 0diff --git a/ci/test/04_install.sh b/ci/test/04_install.sh
 1index e83d78d657..99da8e3b00 100755
 2--- a/ci/test/04_install.sh
 3+++ b/ci/test/04_install.sh
 4@@ -144,7 +144,7 @@ if [[ "${RUN_TIDY}" == "true" ]]; then
 5     CI_EXEC "mkdir -p ${DIR_IWYU}/build/"
 6     CI_EXEC "git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_14 ${DIR_IWYU}/include-what-you-use"
 7     CI_EXEC "cd ${DIR_IWYU}/build && cmake -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-14 ../include-what-you-use"
 8-    CI_EXEC "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
 9+    CI_EXEC_ROOT "cd ${DIR_IWYU}/build && make install $MAKEJOBS"
10   fi
11 fi
12 
13diff --git a/ci/test/05_before_script.sh b/ci/test/05_before_script.sh
14index ef3dff86ca..dd2b43d38b 100755
15--- a/ci/test/05_before_script.sh
16+++ b/ci/test/05_before_script.sh
17@@ -11,6 +11,7 @@ if [ "$CI_OS_NAME" == "macos" ]; then
18   echo > "${HOME}/Library/Application Support/Bitcoin"
19 else
20   CI_EXEC echo \> \$HOME/.bitcoin
21+  CI_EXEC_ROOT echo \> \$HOME/.bitcoin
22 fi
23 
24 CI_EXEC mkdir -p "${DEPENDS_DIR}/SDKs" "${DEPENDS_DIR}/sdk-sources"

which in turn address both #25900 (comment) and #25900 (review).

ci: create and use non-root user for docker image

Running all commands as the root user in the docker image
will change local file permissions in the ci and depends directory.

Add a non-root user to the container and use this user whenever
possible when running docker exec commands.

849f20a6d3

josibake force-pushed on Nov 21, 2022

josibake commented at 5:21 pm on November 21, 2022: member

hey @hebasto , thanks for the testing and suggested changes! I have added both. @MarcoFalke I verified that there is a .bitcoin dummy file created in the container for both users (root/nonroot), so this should be good to go.

changes: git range-diff master 05ce35e 849f20a

hebasto approved

hebasto commented at 10:53 am on November 22, 2022: member

ACK 849f20a6d3e437631a07469d3c4af5faa0aa06ed, tested on Ubuntu 22.04 by running commands as follows:

0git clean -xdff
1MAKEJOBS="-j8" FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh
2MAKEJOBS="-j8" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
3git clean -xdff

maflcko merged this on Nov 22, 2022

maflcko closed this on Nov 22, 2022

in ci/test/04_install.sh:34 in 849f20a6d3

26@@ -27,6 +27,11 @@ export P_CI_DIR="$PWD"
27 
28 if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
29   echo "Creating $DOCKER_NAME_TAG container to run in"
30+  LOCAL_UID=$(id -u)
31+  LOCAL_GID=$(id -g)
32+
33+  # the name isn't important, so long as we use the same UID
34+  LOCAL_USER=nonroot

maflcko commented at 12:07 pm on November 22, 2022:

I don’t think this is true. If a different name is used, it will require root permission to create non-default folders.

DIR_QA_ASSETS=/home/new_user/qa_assets_clean CCACHE_DIR=/home/new_user/ccache_dir_clean CCACHE_SIZE=500M MAKEJOBS="-j4" FILE_ENV="./ci/test/00_setup_env_arm.sh" ./ci/test_run_all.sh

fails with:

0mkdir: cannot create directory ‘/home/new_user/qa_assets_clean’: Permission denied

Presumably because new_user does not exist in the docker (only outside) and is thus root-owned.

maflcko commented at 12:09 pm on November 22, 2022:

If the username from the host is used, this will print a warning:

0useradd: warning: the home directory /home/new_user already exists.

But I guess this is fine?

maflcko commented at 12:45 pm on November 22, 2022:

Sorry, wrong alarm. I think your code is all fine. It is not possible to set DIR_QA_ASSETS, because it is not mounted.

Any folder that is mounted (like CCACHE_DIR), will be created before mounting, see the comment # Create folders that are mounted into the docker. Also, the BASE_ROOT_DIR, which may live outside of /home/nonroot in the docker, will be chown’d to nonroot.

So the code should be good as-is. (I’ll continue testing a bit)

josibake commented at 1:36 pm on November 22, 2022:

thanks for the thorough testing! much appreciated.

sidhujag referenced this in commit fcd81296c3 on Nov 22, 2022

maflcko commented at 10:40 am on November 25, 2022: member

Did some more testing and FILE_ENV="./ci/test/00_setup_env_native_msan.sh" fails with:

0update-alternatives: error: error creating symbolic link '/etc/alternatives/clang++.dpkg-tmp': Permission denied

(not sure if related to this pull)

josibake commented at 1:19 pm on November 25, 2022: member

Did some more testing and FILE_ENV="./ci/test/00_setup_env_native_msan.sh" fails with:
0update-alternatives: error: error creating symbolic link '/etc/alternatives/clang++.dpkg-tmp': Permission denied
(not sure if related to this pull)

good catch! this is related to this pull. I reverted the commit, ran ci without errors, and then reproduced the error running with the commit. fixed in https://github.com/bitcoin/bitcoin/pull/26574

maflcko referenced this in commit c239d3dac9 on Nov 28, 2022

sidhujag referenced this in commit 339228819e on Dec 1, 2022

bitcoin locked this on Nov 25, 2023

josibake deleted the branch on Jan 26, 2024

ci: run docker wrapper with a non-root user #25900

Reviews

Conflicts