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

Docker: Switch from gosu to setpriv in entrypoint.sh #2730

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

aaronkollasch
Copy link
Contributor

@aaronkollasch aaronkollasch commented Sep 22, 2022

Description

This PR switches the su binary in entrypoint.sh from gosu to setpriv (included in Debian). This enables the following improvements:

  • Supplementary groups are not lost if PHOTOPRISM_UID and PHOTOPRISM_GID are set. Supplementary groups are required for hardware transcoding on many devices. (Videos: Improve hardware transcoding on Synology NAS devices #2228)
  • Inherited capabilities from Docker root are no longer passed to photoprism if PHOTOPRISM_UID is set. (This has no effect in typical use, as CapPrm and CapEff are cleared when changing to unprivileged user in Docker.)
  • No longer depends on gosu; removes unnecessary binary from images

setpriv has been available in the util-linux package since Debian 10.0 Buster and Ubuntu 20.04 Focal, so it is present in all supported Docker images. It is also listed as an alternative on the gosu README.

Examples

Before (gosu):

root@220901-bullseye:/photoprism$ gosu "${PHOTOPRISM_UID}" id
uid=1032(user-1032) gid=1032(group-1030) groups=1032(group-1030),33(www-data),44(video),105(davfs2),109(renderd),115(render),937(videodriver),1000(photoprism)

root@220901-bullseye:/photoprism$ gosu "${PHOTOPRISM_UID}:${PHOTOPRISM_GID}" id
uid=1032(user-1032) gid=100(users) groups=100(users)
# setting the GID clears supplementary groups

After (setpriv):

root@220901-bullseye:/photoprism$ /usr/bin/setpriv --reuid "${PHOTOPRISM_UID}" --regid "$(/usr/bin/id -g ${PHOTOPRISM_UID})" --init-groups id
uid=1032(user-1032) gid=1032(group-1030) groups=1032(group-1030),33(www-data),44(video),105(davfs2),109(renderd),115(render),937(videodriver),1000(photoprism)

root@220901-bullseye:/photoprism$ /usr/bin/setpriv --reuid "${PHOTOPRISM_UID}" --regid "${PHOTOPRISM_GID}" --init-groups id
uid=1032(user-1032) gid=100(users) groups=100(users),33(www-data),44(video),105(davfs2),109(renderd),115(render),937(videodriver),1000(photoprism),1032(group-1030)
# supplementary groups are preserved

Acceptance Criteria

  • Features and enhancements are fully implemented so that they can be released at any time without additional work
  • Automated unit and/or acceptance tests have been added to ensure the changes work as expected and to reduce repetitive manual work
  • Contributor License Agreement (CLA) has been signed

Preserves supplementary groups if `PHOTOPRISM_GID` is set.
Removes gosu installation as it is no longer needed.
@CLAassistant
Copy link

CLAassistant commented Sep 22, 2022

CLA assistant check
All committers have signed the CLA.

@lastzero
Copy link
Member

Excellent, I didn't know about setpriv! Although it is a small change (perfect), it might take a while before I can integrate it. We are currently working on multi-user support, which requires all our attention.

Since you seem to have a lot of experience with Unix privileges, feel free to check out the entrypoint-init.sh script as well. With this we install additional dependencies even if the container was not started as root - maybe there is a (better and safer) alternative:

if [[ $(/usr/bin/id -u) == "0" ]]; then
echo "started $DOCKER_TAG as root ($PHOTOPRISM_ARCH-$DOCKER_ENV)"
/bin/bash -c "${INIT_SCRIPT}"
else
echo "started $DOCKER_TAG as uid $(/usr/bin/id -u) ($PHOTOPRISM_ARCH-$DOCKER_ENV)"
/usr/bin/sudo -E "${INIT_SCRIPT}"

@lastzero lastzero self-assigned this Sep 22, 2022
@lastzero lastzero added enhancement Refactoring, improvement or maintenance task docker Docker Images, Build Scripts, Config & Deployment Examples labels Sep 22, 2022
@aaronkollasch
Copy link
Contributor Author

aaronkollasch commented Sep 22, 2022

Awesome! In the meantime, I can just manually edit the entrypoint script and fallback to unaccelerated transcoding if it gets overwritten, no big deal.

I admit the sudo setenv nopasswd entry caught my eye when looking at the Docker container. I haven't found any issues entrypoint-init.sh, and the run-once nature of the make section reduces the potential attack surface, but it's definitely an important script to watch from a security standpoint.
I can't think of a better alternative for installing additional dependencies + not starting as root that doesn't involve docker-compose build with a Dockerfile based on the release image. Also you'd still need root for the recursive chown, if you wanted to keep that.

Side note: while trying to get acceleration to work, I found that the Bookworm image doesn't have intel-opencl-icd available - is that just because it's a Debian testing release?

@lastzero
Copy link
Member

Side note: while trying to get acceleration to work, I found that the Bookworm image doesn't have intel-opencl-icd available - is that just because it's a Debian testing release?

Yes, it seems so - but we are switching to Ubuntu Jammy anyway, since Debian Testing does not receive security updates. The main reason for using Bookworm was the newer versions of FFmpeg, RawTherapee and Darktable which fixed bugs. These are now also available on Ubuntu.

I'll see if I can merge your PR today, since my authentication enhancements have been pushed. They'll take some time to get released anyway - more than 10,000 lines of code.

@lastzero lastzero merged commit 7ab3669 into photoprism:develop Sep 28, 2022
@lastzero lastzero added video Video Formats, Transcoding, FFmpeg, Streaming & Co merged Changes should be tested again after they have been integrated labels Sep 28, 2022
@graciousgrey graciousgrey added the please-test Ready for acceptance test label Oct 23, 2022
@lastzero lastzero added released Available in the stable release and removed please-test Ready for acceptance test labels Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Docker Images, Build Scripts, Config & Deployment Examples enhancement Refactoring, improvement or maintenance task merged Changes should be tested again after they have been integrated released Available in the stable release video Video Formats, Transcoding, FFmpeg, Streaming & Co
Projects
Status: Release 🌈
Development

Successfully merging this pull request may close these issues.

4 participants