Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop root during base image build and use venv #1682

Merged
merged 2 commits into from
Sep 29, 2024

Conversation

kbirger
Copy link
Contributor

@kbirger kbirger commented Sep 29, 2024

Addresses https://github.com/music-assistant/hass-music-assistant/issues/2956

To prevent having to run the container as root, this PR creates as an "app" user and group during the build of the base image (per alpine best practice) and additionally sets /tmp to be writable by that user. The PR also activates the virtual environment created.

Dockerfile.base Outdated Show resolved Hide resolved
@marcelveldt
Copy link
Member

marcelveldt commented Sep 29, 2024

I tested this just now and got the same result as my own testing using a custom user within the container: CIFS mounting doesn't work:

mount: permission denied (are you root?)

So my approach was just to keep the default behavior of running as the default root user (and have a bit of faith in the docker security), drop the privileged and fix the permissions on the venv and temp so that you can potentially run the docker container yourself as different user (but just without the samba provider)...

@marcelveldt
Copy link
Member

Thinking about this more I think this is fine. If a user wants to use the SMB provider to mount a samba share, they can implicitly define --user 0:0 to run the container as the root user again. Just tested that and that works just fine.

installs everything as non-root user and provides
non-root user write access to necessary paths
Copy link
Member

@marcelveldt marcelveldt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@marcelveldt marcelveldt merged commit fcf07cb into music-assistant:main Sep 29, 2024
4 checks passed
@marcelveldt
Copy link
Member

See https://github.com/music-assistant/hass-music-assistant/issues/2956#issuecomment-2381637688

I had to partially revert because it completely broke the HA add-ons (and for now I really want to share the docker layer between regular docker and HA add-on).

I just tested and with the chmods of the workdir and venv, you can now run the container with a custom user.
2.3.0b28 has been released so you can test it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants