linuxserver / docker-jellyfin

GNU General Public License v3.0
642 stars 96 forks source link

Don't chown /data #62

Closed kaysond closed 3 years ago

kaysond commented 4 years ago

Context: I set up all of my services to run as their own user/group (e.g. radarr:radarr, sonarr:sonarr, jellyfin:jellyfin, etc). This makes it easy to control access to different volumes, libraries, configs, etc. If I want multiple services to access a given path, I add the users to the correct group. For example, all of my video content is stored in a Video directory, whose owner is root, and group is video. radarr, sonarr users are all members of the video group. jellyfin is not, because it doesn't need write access to my video files.

The problem is when launching jellyfin, the 30-config cont-init file chowns /data/*.

https://github.com/linuxserver/docker-jellyfin/blob/c5ae0a99a2fdbed28dc2079b72611d3a9ed50ad8/root/etc/cont-init.d/30-config#L17-L18

This script runs as root, so it changes my Video directory owner to jellyfin:jellyfin, which breaks write access for all other services.

This chown behavior doesn't exist in other LSIO containers like radarr or sonarr, and I don't think its necessary as permissions can be managed on the docker host.

I would also suggest that /config and its subdirectories should not be chown'd or chmod'd either because it forces users to adopt specific ownership/permissions.

github-actions[bot] commented 4 years ago

Thanks for opening your first issue here! Be sure to follow the issue template!

aptalca commented 4 years ago
I would also suggest that /config and its subdirectories should not be chown'd or chmod'd either because it forces users to adopt specific ownership/permissions.

Not chowning /data is acceptable, but not chowning /config is not acceptable.

kaysond commented 4 years ago

Not chowning /data is acceptable

Should I make a PR?

not chowning /config is not acceptable.

Why not? Sure, the container won't work if the path isn't accessible, but shouldn't that be managed on the host by the user anyways? The README already says to do that: https://github.com/linuxserver/docker-jellyfin/blob/c5ae0a99a2fdbed28dc2079b72611d3a9ed50ad8/README.md#L169-L180

Silently changing permissions isn't very user friendly. It actually took me months to figure out this is what was causing the problem.

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Artiume commented 3 years ago

one issue you have is on initial install. if you dont create the directories beforehand and allow docker to create the folders, root will be the owner. So do you store your media under data/videos? can i ask why?

Loafdude commented 3 years ago

I also experience this issue. It is very annoying. I don't understand why /data is being chown'd. Making it easier for noobs by stomping all over people's file permissions is unacceptable.

Loafdude commented 3 years ago

one issue you have is on initial install. if you dont create the directories beforehand and allow docker to create the folders, root will be the owner. So do you store your media under data/videos? can i ask why?

I store my data under /data/tvshows /data/movies

kaysond commented 3 years ago

So after dealing with this for a while eventually I just changed my volumes to mount inside the container as /media. This avoids any permission change issues.

Making it easier for noobs by stomping all over people's file permissions is unacceptable.

I mostly agree. I think it makes sense for maybe the minimum required permissions to be set (i.e. on /config only, do a chown but don't chgrp, and do a chmod u+rwx but don't mess with groups). Perhaps something could be added to the log if the permissions aren't set on startup. But I think because these files are written by hand for every lsio repo, it would be painful to make this change consistent across all their containers

Loafdude commented 3 years ago

IMO, If /config does not exist it should have perm/user set upon creation. If /config exists it should not be touched, even if those permissions are inaccessable Users may be accessing these files from other locations and Jellyfin should not change those permissions unilaterally. That said this is less of an issue as not many are directly accessing Jellyfin configs/databases/images

/data should not be touched. I don't see a valid case for this behaviour.

My solution was to comment out the offending lines in /etc/cont-init.d/30-config When I update the container it will overwrite it though.

Roxedus commented 3 years ago

Users may be accessing these files from other locations

Which is why we need to change the permissions, to ensure that the user does not set permissions that prevents Jellyfin from starting

Loafdude commented 3 years ago

Users may be accessing these files from other locations

Which is why we need to change the permissions, to ensure that the user does not set permissions that prevents Jellyfin from starting

There are valid use cases for having modified user/group/mode on /config that would still allow Jellyfin to function.

I also think it is very bad security practice to unilaterally change user/group/mode like this. (Especially all volumes mounted to /data!) Setting system-wide user/group/mode settings on files it did not just create should be outside of Jellyfin's scope as an application.

The system administrator's settings should be respected even if they prevent the application from starting. I believe a log entry such as <file> is inaccessible. Jellyfin is unable to start. in the log is the correct solution.

Regardless, the permission smashing of /data is definitely improper and is a far more pressing issue.

kaysond commented 3 years ago

to ensure that the user does not set permissions that prevents Jellyfin from starting

The problem is that the script doesn't do that. It changes permissions even if its already set in such a way that Jellyfin has access.

aptalca commented 3 years ago

Regardless, the permission smashing of /data is definitely improper and is a far more pressing issue.

Dude, we already said not chowning /data is acceptable, so put your money where your mouth is and submit a PR already. Enough complaining.

Loafdude commented 3 years ago

This was reported 5-6 months ago. It's a 4 line PR. Instead you let it go stale and another user ran into the issue again. I'm not familiar with your code base or conventions but you want me to hack together the init script and submit a 4 line PR. Okay that makes sense.

Also it is not complaining, it is constructive. You've not presented a case for why /config should be chown'd other than but not chowning /config is not acceptable.

Loafdude commented 3 years ago

PR created

github-actions[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kaysond commented 3 years ago

Bumping since the PR got closed.

I think this is still worth addressing in some way, since I'm sure we're not the only users who want to and are capable of managing their own permissions.

Another similar issue came up for me recently where I run services under the group nogroup, for cases when group data access is not necessary. In that case, chowning both /config and /data becomes problematic because it overwrites my group perms (which I use to manage who can edit config, for example) with nogroup.

@Roxedus @aptalca what do you think about adding an env var that prevents any chowning? It would be off by default, so wouldn't affect anyone who doesn't explicitly look for and disable permission settings. It would also be easy to use the same paradigm on other containers.

aptalca commented 3 years ago

Please don't ping me anymore. I said my piece here and other times but nobody seems to care.

Bottom line is, chown for /config folders is a requirement for the majority of the users. You might not need it, but 99% do, so that's not negotiable. /data on the other hand does not need to be chowned here as I said before.

Adding an env var is also a no go, because if we add it here, we would have to add it to our entire fleet and it's not worth the effort for the handful of people it would benefit.

If you don't want /config chowned, feel free to customize as described here: https://www.linuxserver.io/blog/2019-09-14-customizing-our-containers

kaysond commented 3 years ago

Please don't ping me anymore.

Never have before...

I said my piece here and other times but nobody seems to care.

Not true. People just disagree with you, which they're absolutely entitled to. I'm trying to make an effort to address your objection.

If you don't want /config chowned, feel free to customize as described here: https://www.linuxserver.io/blog/2019-09-14-customizing-our-containers

Yeah I'll probably just make a mod that just sed's all the chown abc:abc out so I can apply it to multiple containers.

kaysond commented 3 years ago

Going to close this since its made it to master, but I'll add a comment here when I get around to making a mod that nukes permission touching

dannymichel commented 2 years ago

Going to close this since its made it to master, but I'll add a comment here when I get around to making a mod that nukes permission touching

But is it fixed though?

kaysond commented 2 years ago

@dannymichel yes in that /data won't get chowned in the latest containers

Still haven't gotten around to making that mod tho

XtremeOwnageDotCom commented 2 years ago

Well....as I am having issues where various linuxserver based containers are trying to chown nfs mounts.... id say, this one wasn't fixed.

kaysond commented 2 years ago

Well....as I am having issues where various linuxserver based containers are trying to chown nfs mounts.... id say, this one wasn't fixed.

@XtremeOwnageDotCom are you squashing root? That would cause all chowns/chmods to fail. #169 is the right approach here, and would solve the issue with nfs mounts.

XtremeOwnageDotCom commented 2 years ago

I don't recall what my issue originally stemmed from-

But, in my case, I have NFS mounts exposed by my NAS, which also, used specific UID/GIDs for permissions.

The containers, would execute chown for the mounts, which, actually would work. However, this then, resulted in the docker server, unable to access anything over the mount, because it was just chowned to an invalid GID/UID... The end result would be a broken mount until I went on the NAS, updated the UID/GID to be the correct values.

I believe the fix in my case, was to verify containers all specified the correct UID/GID variables, when possible (Not all containers expose those fields), and on the NFS side, I did a mapall UID/GID, to map all access back to the proper UID/GID, in the event the container did not obey UID/GID/USER_ID/GROUP_ID/etc environment variables.