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

Dockerfiles: Properly set up non-root users #1265

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

RyanGlScott
Copy link
Contributor

Previously, the crux-{llvm,mir} Docker images were set up to use root accounts, and all files that were created in the Docker images were given root permissions. This is impractical for running rootless container environments, as noted in #1261.

This rewrites the images to set the default users to unprivileges crux-{llvm,mir} users. This is surprisingly involved, and it requires a fair bit of refactoring to accomplish:

  1. We cannot easily base the crux-mir image on the official rust image, as the latter has cargo, rustup, and related tools installed under a root account. Instead, we change to an unprivileged user and install these Rust tools using rustup.sh's installer script.
  2. Previously, the crux-llvm image was supporting Unicode character printing by changing the locale, which requires root access to accomplish. This is overkill, however, as it suffices to simply set the LANG and LC_ALL environment variables. I've opted for the latter approach, which is much simpler and avoids the need for root access.
  3. I needed to reorganize the order of commands in the base layers of each image in order to ensure that the relevant files are chowned from the unprivileged user accounts.

Fixes #1261.

Previously, the `crux-{llvm,mir}` Docker images were set up to use `root`
accounts, and all files that were created in the Docker images were given
`root` permissions. This is impractical for running rootless container
environments, as noted in #1261.

This rewrites the images to set the default users to unprivileges
`crux-{llvm,mir}` users. This is surprisingly involved, and it requires a fair
bit of refactoring to accomplish:

1. We cannot easily base the `crux-mir` image on the official `rust` image,
   as the latter has `cargo`, `rustup`, and related tools installed under a
   `root` account. Instead, we change to an unprivileged user and install these
   Rust tools using rustup.sh's installer script.
2. Previously, the `crux-llvm` image was supporting Unicode character printing
   by changing the locale, which requires root access to accomplish. This is
   overkill, however, as it suffices to simply set the `LANG` and `LC_ALL`
   environment variables. I've opted for the latter approach, which is much
   simpler and avoids the need for root access.
3. I needed to reorganize the order of commands in the `base` layers of each
   image in order to ensure that the relevant files are `chown`ed from the
   unprivileged user accounts.

Fixes #1261.
@RyanGlScott
Copy link
Contributor Author

Successful Docker CI jobs for crux-llvm and crux-mir.

Copy link
Member

@kquick kquick left a comment

Choose a reason for hiding this comment

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

I think these could be a bit more efficient (each ADD and COPY creates a layer, so we can compress and re-order), but that's unrelated to the changes being effected here, so we can address that separately.

@RyanGlScott RyanGlScott merged commit d0c27a1 into master Oct 28, 2024
47 checks passed
@RyanGlScott RyanGlScott deleted the T1261-more-dockerfile-improvements branch October 28, 2024 18:35
Copy link
Contributor

@sauclovian-g sauclovian-g left a comment

Choose a reason for hiding this comment

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

There's stuff you might want to clean up further in the future, like it sets LANG and LC_ALL more than once. Also I'm surprised you need to put /usr/local/lib in LD_LIBRARY_PATH (or maybe you don't actually).

(also if C.UTF-8 works, that's great, I just remember complaining to I think Debian when it didn't work and being told that's not a bug.)

@RyanGlScott
Copy link
Contributor Author

There's stuff you might want to clean up further in the future, like it sets LANG and LC_ALL more than once.

Does it? I need to set it once per stage, but my understanding is that you do in fact need to set it separately for each stage, given that the stages don't inherit from each other. (I might be misunderstanding something about how Docker works here, however.)

Also I'm surprised you need to put /usr/local/lib in LD_LIBRARY_PATH (or maybe you don't actually).

Truth be told, I'm a bit skeptical about that as well. I decided not to touch that part of the crux-llvm Docker setup, but I share your belief that this step is likely unnecessary.

(also if C.UTF-8 works, that's great, I just remember complaining to I think Debian when it didn't work and being told that's not a bug.)

I've successfully used this before on other Ubuntu-based Docker images, so I feel pretty confident about using it here. I can't speak to how portable it would be in other Linux distros, however.

@sauclovian-g
Copy link
Contributor

Hrm, maybe you do. N/M.

C.UTF-8 ought to work. It just didn't at one point in the relatively recent past (2018ish?)

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

Successfully merging this pull request may close these issues.

Docker images: Don't default to root user
4 participants