JACOBSMILE / tmodloader1.4

An easy to configure Docker Image for tModLoader 1.4 servers.
Other
120 stars 29 forks source link

Everything runs as root - that's a serious security concern #12

Closed FlorentLM closed 1 year ago

FlorentLM commented 1 year ago

Hi there,

Thanks for putting this repo together, all these older repos were indeed full of good stuff and it's extremely nice to see them coming together! :)

One thing that is worrysome though, is that everything runs as root - this is quite a substantial security concern, and is even advised against in SteamCMD's documentation.

I made a rootless version which seems to work fine, and should be much safer, feel free to look at the fork, I'll create a pull request once I've ironed out some details :)

Cheers

JACOBSMILE commented 1 year ago

Hi, thanks for alerting me. I appreciate the help on your fork, I'll be happy to review the PR when you get it put together.

JACOBSMILE commented 1 year ago

@FlorentLM I have published a Pull Request (#14) integrating some of my recent changes in my updates in PR #13, and your fork. I have not merged it yet since you had mentioned you were ironing out some details. I also want to make sure you receive proper contributor credit, which I think you will since I merged in from both our branches and resolved the conflicts. Please take a look when you get a chance and let me know before I merge it.

Appreciate the help!

FlorentLM commented 1 year ago

Hey hey,

Thanks again for adding my changes :) I just pushed some more tweaks to my fork, here's a quick summary:

NOTE: As per the recommended approach, users must chown the host folder to 456:456 before starting the container, otherwise the container might not be able to write its stuff in it.

Example:

chown -R 456:456 /path/on/the/host/terraria
docker run -v /path/on/the/host/terraria:/data 

This should also make things easier if one wants to run the docker daemon as rootless and use subuids

JACOBSMILE commented 1 year ago

@FlorentLM Do you mind making a pull request, which is also updated with the latest master branch? I'm also going to have to redirect users to map their directories to a different spot internal to the container (again), which will make updating a bit trickier, but I believe my update notice messages will help to bridge that gap.

Appreciate the continued help!