Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run as limited user inside docker #445

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

Spacetown
Copy link
Member

Extend test with read only output directory.

@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #445 (32e4851) into master (15a108e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage   95.49%   95.52%   +0.02%     
==========================================
  Files          20       20              
  Lines        2444     2458      +14     
  Branches      420      420              
==========================================
+ Hits         2334     2348      +14     
  Misses         48       48              
  Partials       62       62              
Flag Coverage Δ
ubuntu-18.04 95.03% <100.00%> (+0.02%) ⬆️
windows-2019 95.11% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gcovr/tests/test_args.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15a108e...32e4851. Read the comment docs.

@Spacetown Spacetown force-pushed the limited_user_inside_docker branch 4 times, most recently from 42411bc to a486f3a Compare December 11, 2020 22:33
@Spacetown
Copy link
Member Author

From a colleague I got the information how to use a different user in docker.
Now also a read only output directory is checked under ubuntu and inside the docker container.

Copy link
Member

@latk latk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a great change, especially for the extra tests this enables. However, these changes do have the potential to damage a developer's system, so extra discussion is needed.

gcovr/tests/test_args.py Show resolved Hide resolved
USER docker:docker

CMD ( echo docker | sudo -S -p "Running pip with sudo" $PYTHON -m pip install -e . ) && \
( echo docker | sudo -S -p "Running chmod with sudo" chown -R docker:docker /gcovr ) && \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid the chmod will affect the user ID of the file on the host system, potentially making the project inaccessible for the user. Since you create a group+user, they will get the UID=1000, GID=1000. This happens to be the matching UID/GID for the first user on most Linux systems. This will be the completely wrong user ID for other users, especially for people working on shared servers (not that unusual in academia). This could render the gcovr project directory inaccessible and undeletable for such users after running a test. Of course being able to run Docker is effective root access so they could restore access with e.g. docker run --rm -v "$(pwd)":/gcovr ubuntu chmod -R "$(id -u):$(id -g)" /gcovr, but that is rather non-obvious.

Idea: can we inject the appropriate numeric IDs at runtime through environment variables? Or would Docker's usermap feature help? I've never used it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I forgot this. The chmod was needed because of the option --archive_artifacts which is used on GitHub actions. I`ll check this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked this point. If I run following command inside docker I get the owner docker:

$ touch xy
$ dir -l xy
-rw-r--r-- 1 docker docker 0 Dec 14 20:00 xy

Outside docker, the owner is the user running the docker instance:

Snoopy@Admins-Air gcovr % ls -l xy
-rw-r--r--+ 1 Snoopy  wheel  0 Dec 14 21:00 xy

Also the file mode isn't changed in the host system.

It seems, that the chmod doesn't change the

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you try showing the file with ls -ln to show the numerical user/group IDs? I ran tests on Ubuntu host + container, and the two systems always showed the same numerical IDs in the file permissions. Of course the user name was different, but that's just due to different /etc/passwd contents. So when running as user 1000 in the container host would see user 1000. You might therefore be able to provoke breakage by choosing a different uid in the Docker container, e.g. with useradd ... -u 1234.

It could be that things work differently on Macos, since it doesn't support Docker natively but requires a virtualization layer. I would have expected to already see such breakage on your system, since Macos should start numbering UIDs at 501, not at 1000.

This article suggests telling the container about the UID via an environment variable, and creating an entrypoint that dynamically creates a user with appropriate UID: https://denibertovic.com/posts/handling-permissions-with-docker-volumes/ – this might actually work.

<rant> The real problem is that on Linux, Docker spawns container processes from the Docker Daemon which runs as root, and doesn't really know about the user who “started” the container. The docker command is just a client for dockerd. I understand why that's necessary, but the entire architecture is insane and leads to endless problems. I looked at the user remap / userns feature and it's totally unsuitable for us, since people wanting to run the container would have to reconfigure their Docker Daemon. </rant>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside docker I get following output:

ls -l test.xy
-rw-r--r-- 1 docker docker 0 Dec 17 20:37 test.xy
ls -ln test.xy
-rw-r--r-- 1 1000 1000 0 Dec 17 20:37 test.xy

and inside MacOs:

Snoopy@Admins-Air gcovr % ls -l test.xy      
-rw-r--r--@ 1 Snoopy  wheel  0 Dec 17 21:37 test.xy
Snoopy@Admins-Air gcovr % ls -ln test.xy
-rw-r--r--@ 1 503  0  0 Dec 17 21:37 test.xy

It seems, that on MacOs the files from docker are mapped to the user running the container.

I added the effective user and group id to the container. Inside the container the group and user are mapped to this ids. At the moment I've added the ls commands to the makefile to get the data from the actions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gid from the host is already used inside docker, therefore I removed the gid again.
Because of the issue actions/setup-python#171 I've switched to the last commit to run tests with pypy3.

gcovr/tests/test_args.py Show resolved Hide resolved
@Spacetown Spacetown force-pushed the limited_user_inside_docker branch 4 times, most recently from e6a65d0 to d1f53f0 Compare December 21, 2020 20:32
@Spacetown Spacetown force-pushed the limited_user_inside_docker branch 2 times, most recently from 7b2aa32 to 3583442 Compare January 7, 2021 18:44
@Spacetown Spacetown added the QA related to testing, build infrastructure, etc label Jan 20, 2021
@Spacetown Spacetown requested a review from latk January 28, 2021 19:28
Copy link
Member

@latk latk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested on my system and it seems to work fine. Thank you!

@Spacetown Spacetown force-pushed the limited_user_inside_docker branch 2 times, most recently from 8c3166b to d5c75f2 Compare February 1, 2021 21:27
- Extend test with read only output directory (only inside docker).
@Spacetown Spacetown merged commit 136b154 into gcovr:master Feb 1, 2021
@Spacetown Spacetown deleted the limited_user_inside_docker branch February 1, 2021 21:37
@Spacetown Spacetown added this to the 4.3 milestone Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA related to testing, build infrastructure, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants