cjones-teradici commented 2 years ago

Here is a pull request to fix some of the issues. More to follow

cjones-teradici commented 2 years ago

Hello.

This is not a docker error. Why are we “chown”ing files to a particular user. This is now UID/GUID dependentant code and should not be done in this fashion.

Regards, Chris

On May 24, 2022, at 9:11 PM, Fer @.**@.>> wrote:

@klahnen commented on this pull request.

In Dockerfilehttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FThreagile%2Fthreagile%2Fpull%2F21%23discussion_r881199487&data=05%7C01%7Ccjones%40teradici.com%7C7b8186b9b2fe465b674d08da3e04aed2%7C715c7afb027a4b6e9a60dc385a62cf18%7C0%7C0%7C637890487125050011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7Oi1i3f6HfR6sJiXiUyYpt2dCch%2BYvdKHaPfl1V2MZY%3D&reserved=0:

@@ -56,11 +50,8 @@ COPY --from=build /app/demo/stub/threagile.yaml /app/threagile-stub-model.yaml

RUN mkdir /data

-RUN chown -R 1000:1000 /app /data

question: I assume you removed these lines because you were experiencing some errors while running the container.

What errors were you experiencing?
What version of docker were you running? (seems like an error in docker-for-linux)

— Reply to this email directly, view it on GitHubhttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FThreagile%2Fthreagile%2Fpull%2F21%23pullrequestreview-984189300&data=05%7C01%7Ccjones%40teradici.com%7C7b8186b9b2fe465b674d08da3e04aed2%7C715c7afb027a4b6e9a60dc385a62cf18%7C0%7C0%7C637890487125050011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pRUjRMpZfgT2EVSFf6VVmWUXNZAEkocABYXmCmafhkA%3D&reserved=0, or unsubscribehttps://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAUM4M4FFF6IWGE4NHPI437DVLWSADANCNFSM5CRFC2FA&data=05%7C01%7Ccjones%40teradici.com%7C7b8186b9b2fe465b674d08da3e04aed2%7C715c7afb027a4b6e9a60dc385a62cf18%7C0%7C0%7C637890487125050011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9H9M7yVna5b8KvShIqowt1HtbZVVnsNwIJj2cH1dVhM%3D&reserved=0. You are receiving this because you authored the thread.Message ID: @.***>

cschneider4711 commented 2 years ago

It's good security practice to run processes in containers as some other low-privileged user instead of root (see for example https://medium.com/@mccode/processes-in-containers-should-not-run-as-root-2feae3f0df3b ). Possibly it would be clearer to also add group/user inside the Dockerfile via groupadd/useradd to make this more obvious, instead of only using the USER directive to switch to a non-root user.

cjones-teradici commented 2 years ago

Yes. I 100% agree, but you’re doing it wrong!

You should be using userne-remap!

Setting it to an arbitrary UID/GUID is more dangerous. This user could have sudo/root privileges.

We should be doing something like this:

sudo mkdir -p $DOCKER_DIR

Check docker configuration file existence and minimum content

[[ -f $DOCKER_DAEMON_FILE ]] || echo "{}" | sudo tee -a $DOCKER_DAEMON_FILE [[ -s $DOCKER_DAEMON_FILE ]] || echo "{}" | sudo tee -a $DOCKER_DAEMON_FILE