Don't run docker containers as root by jeremyestein · Pull Request #400 · SAFEHR-data/PIXL

jeremyestein · 2024-05-08T14:27:47Z

Fixes #234. Waiting for testing on GAE before merging.

Note the addition of two new variables PIXL_USER_UID and PIXL_USER_GID

Firstly merge all the Dockerfiles for images that we control (imaging, export, hasher) to make this process easier.

Run all our python containers as the user/group pixl, which we create as part of the build process, using the UID/GID as specified in the config.

Export API mounts export dir read-only as it doesn't need to write any more.

Document how the host must be set up for this to work.

Do same for orthanc images.

avoid repetition.

codecov · 2024-05-08T14:30:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.17%. Comparing base (6435ebb) to head (87df99c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #400      +/-   ##
==========================================
- Coverage   87.38%   85.17%   -2.22%     
==========================================
  Files          76       72       -4     
  Lines        3386     3116     -270     
==========================================
- Hits         2959     2654     -305     
- Misses        427      462      +35

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

run as non-root users.

fixed user ID.

the correct env files in. No need to do it here as well, and it was failing due to lack of build args.

…n set file permissions correctly for the export directory, which is needed so that it can be deleted from inside the container.

creation later on

sense to delete stuff afterwards from the host side rather than the container side.

export variables any more.

enforce this to avoid any weird surprises.

stefpiatek · 2024-06-14T16:10:46Z

Had to run using docker group GID because all mounts will preserve the group that wrote them.

Added as a secondary group and as its just a file permission I think that'd be fine.

Example of permission error for a directory:

pixl_dev-orthanc-anon-1  | E0614 14:32:09.175160             MAIN main.cpp:2123] Uncaught exception, stopping now: [boost::filesystem::directory_iterator::construct: Permission denied: "/run/secrets"]

README.md

docker/hasher-api/Dockerfile

Co-authored-by: Milan Malfait <m.malfait@ucl.ac.uk>

Following good practice: https://github.com/hadolint/hadolint/wiki/DL3059

# Conflicts: # .github/workflows/main.yml # docker/hasher-api/Dockerfile # docker/imaging-api/Dockerfile # docker/orthanc-anon/Dockerfile # docker/orthanc/Dockerfile # docker/pixl-python/Dockerfile # pixl_imaging/tests/docker-compose.yml # test/conftest.py

jeremyestein · 2024-12-18T18:49:03Z

docker/orthanc/Dockerfile

+COPY --chown=orthanc:orthanc ./orthanc/orthanc-anon/plugin/pixl.py /etc/orthanc/pixl.py
+COPY --chown=orthanc:orthanc ./orthanc/orthanc-anon/config /run/secrets
+
 RUN sed -i "s/\${ORTHANC_CONCURRENT_JOBS}/${ORTHANC_CONCURRENT_JOBS:-5}/g" /run/secrets/orthanc.json


ORTHANC_CONCURRENT_JOBS doesn't seem to be declared as a build ARG for pixl_orthanc_anon. Is it just reverting to the given default value?

jeremyestein · 2024-12-18T18:51:33Z

Have split the refactoring side of this into #583 , which could possibly be merged with less risk than this PR. If we do decide to merge this, I think the fixup commit in the other branch (98fa689) should be applied here too.

stefpiatek · 2024-12-19T17:08:59Z

Hmm I'd look for missing env variables for the failure

Fixes #588 by de-duping the orthanc and PIXL dockerfiles. This was factored out from PR #400 which had become too big. * Merge three pixl python dockerfiles into one multi-stage dockerfile to avoid repetition. * Merge orthanc dockerfiles, downloading dicom spec at a more separate stage. * Specify healthcheck command in only one place

jeremyestein added 4 commits May 8, 2024 14:44

Make packages easier to read

93e99d5

Remove unneeded packages

4b1f8fa

Merge three pixl python docker images into one multi-stage dockerfile to

d087520

avoid repetition.

Specify healthcheck command in only one place

1e1c18e

jeremyestein added 25 commits May 8, 2024 16:17

De-dupe the pre-reqs installation code using a build arg

63d6edb

De-dupe the actual install as well

46a08e0

Add (failing) test for the condition we are aiming for: containers to

64fcbc5

run as non-root users.

Run as user pixl, which we have to create inside the image with a

0d5cb39

fixed user ID.

The system test already builds the required docker images, and passes

397cb68

the correct env files in. No need to do it here as well, and it was failing due to lack of build args.

Semi-WIP: Create pixl user+group on the GHA test runner so that we ca…

d7fa46e

…n set file permissions correctly for the export directory, which is needed so that it can be deleted from inside the container.

Fix variable usage and exports dir location

d8525b1

More debugging

6facef9

debug

55db93c

debug

6f7c8dd

split build and up

0ff9148

Create user home dir in the GHA script to avoid error from failed

cce26a5

creation later on

Why is pytest not found? Some kind of env change caused by sudo?

1e3ed88

Preserve path when running sudo

8abcb5f

Run tests as normal runner user

de4e1e9

Don't see why this needs to have docker group

5736f30

Debugging for perm failure on file deletes

09f37ea

Fix scope error

0edcf17

OK maybe this was needed after all

d0201d8

Since CLI creates everything in exports dir these days, it makes more

077a304

sense to delete stuff afterwards from the host side rather than the container side.

Try not creating home dir for pixl - what is even using this?

cd93adc

and therefore I can't do this any more

c0c0d96

Remove debugging

9507816

Document the reason for the permissions setup better. Don't need to

bd5cf84

export variables any more.

If Export API is believed to only read from the export dir, then let's

de2ea9d

enforce this to avoid any weird surprises.

Don't think this is needed since we moved this code to the export-api

b4d2526

jeremyestein mentioned this pull request May 23, 2024

Run as non-root user "pixl" where possible [force-system-test] #345

Closed

2 tasks

stefpiatek added 10 commits June 14, 2024 14:27

Try adding orthanc to PIXL group

cca7b93

Try adding orthanc to PIXL group

5925520

Try adding orthanc to PIXL group

3437005

Try adding orthanc to PIXL group

905e741

Change owner of pixl file

19be6c7

Try setting pixl user before copying

4c39072

Change owner of python projects on copy

32ca6bb

Change owner of python projects on copy

81eabb7

Change owner of python projects on copy

21ccb13

Merge branch 'main' into jeremy/docker-uid-new

9ef77e7

stefpiatek and others added 3 commits June 14, 2024 17:19

Define wait for images to be exported in conftest

ee1b97c

Add group ID to test build args

47822a9

Merge branch 'main' into jeremy/docker-uid-new

242aacf

milanmlft approved these changes Jul 4, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

docker/hasher-api/Dockerfile Show resolved Hide resolved

stefpiatek and others added 8 commits July 18, 2024 11:23

Update README.md

6e945c3

Co-authored-by: Milan Malfait <m.malfait@ucl.ac.uk>

Merge branch 'main' into jeremy/docker-uid-new

c7b2711

Consolidate RUN commands

521b1cd

Following good practice: https://github.com/hadolint/hadolint/wiki/DL3059

Merge branch 'main' into jeremy/docker-uid-new

653a921

Add @dram1964's instructions for setting up a PIXL user on GAE

5d1937d

Merge branch 'main' into jeremy/docker-uid-new

87df99c

jeremyestein mentioned this pull request Dec 18, 2024

Refactor Python and Orthanc Dockerfiles #583

Merged

13 tasks

jeremyestein commented Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't run docker containers as root#400

Don't run docker containers as root#400
jeremyestein wants to merge 82 commits intomainfrom
jeremy/docker-uid-new

jeremyestein commented May 8, 2024 •

edited

Loading

Uh oh!

codecov bot commented May 8, 2024 •

edited

Loading

Uh oh!

stefpiatek commented Jun 14, 2024

Uh oh!

Uh oh!

Uh oh!

jeremyestein Dec 18, 2024

Uh oh!

jeremyestein commented Dec 18, 2024

Uh oh!

stefpiatek commented Dec 19, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jeremyestein commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stefpiatek commented Jun 14, 2024

Uh oh!

Uh oh!

Uh oh!

jeremyestein Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

jeremyestein commented Dec 18, 2024

Uh oh!

stefpiatek commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jeremyestein commented May 8, 2024 •

edited

Loading

codecov bot commented May 8, 2024 •

edited

Loading

stefpiatek commented Dec 19, 2024 •

edited

Loading