Docker: Use group 'squeezeboxserver' instead of 'nogroup' for file permissions

[osnieh]

I have a suggestion for a more or less cosmetic change for the docker image:

• In Dockerfile the group of the user 'squeezeboxserver' is set to $PGID, but later in the chown command the group 'nogroup' is used.
• In start-container.sh the command groupmod -o -g "$PGID" nogroup has the side effect, that also the user 'nobody' is modified and gets access to all files with the group $PGID.
  On the other hand, the user 'squeezeboxserver' still has group 100 (set in Dockerfile) as standard group, because this will not be changed by the startscript.

I think it would be cleaner to use the group 'squeezeboxserver' (that is impicit created in Dockerfile by the command useradd) instead of the the group 'nogroup'.

Is this a good idea? Then a will create a pull request. otherwise I won't do it.

[michaelherger]

I'm not sure I follow (I'm no Docker expert!). But when I hear things like "user X gets access to all files", then some alarms start ringing in my head! Would that impact files outside the container? In this case this would be a security issue, much more than cosmetic. And I'd be all for fixing it. But I'm a little confused with groups inside and outside the container...

[smilerish]

This seems closely related to the issue #16 which is still outstanding after four years! It's also the reason I came looking here because the Docker container seems to be useless without this fix. (I'm not sure how anyone else is using it unless they're working around it somehow?)

In principle, I agree with @osnieh's suggestion. I don't understand why nogroup is created and also the lms user isn't added to that group, so the server doesn't get the necessary permissions to access files based on $PGID.

I've never submitted a pull request before, but I'm willing to learn if I can help to fix this 🙂

[michaelherger]

What OS would you be running your Docker on? As you said it would be surprising if that worked for thousands of users when it was really, really broken...

[smilerish]

OS isn't the issue (I'm running Arch on a Raspberry Pi). It's the way I have permissions set on my music library.

The folder has group read-write access, and I set the PGID to the group ID of that group. But it doesn't work because, as this thread and issue #16 identifies, the squeezeboxserver user isn't actually added to that group within the Docker container.

I can only assume that everyone else is relying on PUID for access, but I don't want to do that (nor should I have to if the container were working as documented) for a number of reasons.

[michaelherger]

Would you confirm @osnieh's suggestion (public/9.1...osnieh:slimserver-platforms:Docker-change-permissions-of-user-running-the-server) would fix your issue? If so, could @osnieh please provide a pull request? Thanks!

[smilerish]

I've managed to build a Docker image using @osnieh amendments and confirm it does fix this problem in my case! I've provided some additional feedback below.

[osnieh]

These are two issues:

1. By modifying 'nogroup' (line 9 of start-container.sh), you also modify the user 'nobody' and and set it's standard group to $PGID. This is normally 100 (= users) which is set in Dockerfile. This could be a problem if there is a process running in the context of 'nobody'.

2. The user 'squeezeboxserver' has the userid $PUID and the groupid 100. This groupid is also set in Dockerfile and is not changed in start-container.sh. So if you want to set the GID of the server to XYZ to access your music library that will not work, because the server is always running with group 100.
   In addition the chown (line 12 of start-container.sh) sets file permissions of /config and /playlist to $PUID:$PGID, but the server will always create new files (e.g. playlists) with groupid 100.
   This will only occur, if you set the environtment variable PGID in the docker command to something different as 100.

My suggestions is:

• In Dockerfile add the user without home and login and without modifying uid and gid:
  RUN useradd -M -d /nonexistent -s /usr/sbin/nologin -G audio squeezeboxserver
• In start-container.sh:
  In line 9 replace nogroup by squeezeboxserver: groupmod -o -g "$PGID" squeezeboxserver
  In line 12 replace nogroup by squeezeboxserver: chown -R squeezeboxserver:squeezeboxserver /config /playlist
  In line 23 add shell: su squeezeboxserver -s /bin/sh -c '...'

And maybe we can rename squeezeboxserver to lyrion :-)

[osnieh]

This would look like this: https://github.com/osnieh/slimserver-platforms/tree/Docker-change-permissions-of-user-running-the-server

[smilerish]

Thanks for doing this, @osnieh! I've been able to build a Docker image using your changes, and it solves the problem I had.

Some questions/feedback:

1. In Dockerfile, should the following line also be updated to use the squeezeboxserver group?
   RUN chown -R squeezeboxserver:nogroup /config /playlist && chmod -R a+rX /lms
2. In start-container.sh, why have you amended the last line to add -s /bin/sh? I don't see how it's related to this issue. If it is necessary for some other reason, would it not be neater if this change were incorporated into a separate branch/pull request?

[osnieh]

To the First Point: Yes you are right, but at the end it doesn‘t matter, because the permissions are finally set in start-container.sh.

And second there are two changes in my branch, that have nothing to do with the issue:

1. I do not set uid/gid of the new user to 99:100. So it will get 1000:1000 at creation and this is not changed.
2. I set the homedir to /nonexistent and shell to /usr/sbin/nologin (this is similar to adduser --system). Because su needs a shell I had to add -s /bin/sh in the last line of start-container.sh

I think we should not change to much at the same time, so I will remove this two changes before creating the pull request. And then we can discuss if we want to change this in another pull request or not.

[smilerish]

Oh I understand now! Okay, I agree that those changes shouldn't be included here. I have some more ideas for improving the Docker image - perhaps we should start a separate discussion or issue for this?

[osnieh]

Now I have implemented the minimum change, that solves this issue (2 lines in Dockerfile and 2 lines in startcontainer.sh)

@smilerish : My build looks good. Could you please also do a test with this version.

[smilerish]

Confirmed that it's working - at least as far as file permissions are concerned.

I've since noticed that LMS is issuing errors/warnings that Slim/Utils/OS/Utils.pm fails to compile. I'll look into this, but I don't think it's caused by these changes.

Scratch that - it's because I built the Docker image with v9.0.1 of lms but v9.1 has moved Docker.pm from the platforms repo to the core repo.

[osnieh]

Here is the pull request: #85

[osnieh]

Hello @michaelherger,

@smilerish suggested to collect ideas for the docker image.
Is the discussion section here the right place for this?

BR, Oliver

[michaelherger]

I'd prefer the forum, as it sees a broader audience. Most of this community's activity happens on https://forums.lyrion.org.

[osnieh]

@smilerish, fyi: As suggested above, I have started a discussion in the dev forum: https://forums.lyrion.org/forum/developer-forums/developers/1744630-lms-ideas-for-the-docker-image