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

Build unprivileged minimal container image #29350

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

AndrewFerr
Copy link
Member

  • Base the Docker image on gcr.io/distroless
  • Use nginx-unprivileged instead of tweaking ownership manually

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

- Base the Docker image on gcr.io/distroless
- Use nginx-unprivileged instead of tweaking ownership manually
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

This seems to conflict with #29346 which relies on the entrypoint.d behaviour of the upstream nginx image

Comment on lines +44 to +64
COPY --from=nginx \
/lib/${LIBARCH}-linux-gnu/ld-2.31.so \
/lib/${LIBARCH}-linux-gnu/libc-2.31.so \
/lib/${LIBARCH}-linux-gnu/libcrypt.so.1 \
/lib/${LIBARCH}-linux-gnu/libdl-2.31.so \
/lib/${LIBARCH}-linux-gnu/libpthread-2.31.so \
/lib/${LIBARCH}-linux-gnu/libz.so.1 \
/lib/${LIBARCH}-linux-gnu/

COPY --from=nginx \
/usr/lib/${LIBARCH}-linux-gnu/libcrypto.so.1.1 \
/usr/lib/${LIBARCH}-linux-gnu/libpcre2-8.so.0 \
/usr/lib/${LIBARCH}-linux-gnu/libssl.so.1.1 \
/usr/lib/${LIBARCH}-linux-gnu/

COPY --from=nginx /usr/sbin/nginx /usr/sbin/nginx
COPY --from=nginx /usr/share/nginx/html /usr/share/nginx/html
COPY --from=nginx /etc/nginx /etc/nginx
COPY --from=nginx /var/log/nginx /var/log/nginx
Copy link
Member

Choose a reason for hiding this comment

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

This looks pretty unmaintainable

Copy link
Member Author

Choose a reason for hiding this comment

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

There's unfortunately no official image for distroless Nginx to use as a base. There exist third-party images, but that would add a potentially unreliable dependency. So the next best idea is to build it here.

What could help with stability is to lock down the Nginx image to a specific release. It also should be possible to write a script that checks which libraries are used by the base Nginx process & update this Dockerfile accordingly.

Copy link
Member

@t3chguy t3chguy Feb 24, 2025

Choose a reason for hiding this comment

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

It also should be possible to write a script that checks which libraries are used by the base Nginx process & update this Dockerfile accordingly.

Couldn't the same script just set up a chroot in the originating image and copy that across to the app image? COPY --from=nginx /everything-nginx-needs/ /

@t3chguy
Copy link
Member

t3chguy commented Feb 24, 2025

Please refrain from force pushing once you have put a PR up for review, it breaks any ability to do delta reviews. The merge queue enforces squash merges.

@t3chguy
Copy link
Member

t3chguy commented Feb 24, 2025

Note, as of #29346 which is about to land there are a number of incompatibilities

  1. Image needs sh
  2. Image needs jq
  3. Image needs moreutils/sponge
  4. Image needs entrypoint which calls docker-entrypoint.d

Also worth noting it'd be criminal to not include wget like we have today to enable healthchecks without needing an external daemon, e.g. with a stock docker-compose.

In the near future the image will also need curl/wget for an additional entrypoint anyhow.

Comment on lines +34 to +36
# Run a no-op action of nginx to run entrypoint scripts that may tweak config files
USER nginx
RUN ["nginx", "-t"]
Copy link
Member

Choose a reason for hiding this comment

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

This won't work as this isn't the final image, yet uses runtime environment variables.

https://github.com/element-hq/element-web/blob/develop/docs/install.md#docker specifies ELEMENT_WEB_PORT runtime envvar, which is consumed by the nginx entrypoint scripts which tweak the nginx config files, if this is run only at build time then it will be broken.

Comment on lines 71 to 72
# HTTP listen port
ENV ELEMENT_WEB_PORT=80
Copy link
Member

Choose a reason for hiding this comment

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

@AndrewFerr
Copy link
Member Author

  1. Image needs sh

That is at odds with using a distroless image, so putting this PR in draft for now.

In the meantime, I split out the usage of nginx-unprivileged into its own PR: #29353

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

You haven't said what benefit this gives or linked to an issue. Distroless seems nice in principle but I agree with Michael, having to extract the libc from an ubuntu image to run it seems very prone to breakage. If we're doing this, I'd vote for building on a dedicated distroless nginx base docker image, not trying to make one ourselves as part of element-web.

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.

3 participants