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

Fix SELinux labels to allow shared use. #962

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

Alexhuszagh
Copy link
Contributor

@Alexhuszagh Alexhuszagh commented Jul 18, 2022

Ensure that the volumes are not mounted as private, unshared volumes since we might mount with the host filesystem. This also fixes permissions issues with reading data from a mounted volume using a rootless container engine.

Fixes a bug introduced in #251.
Closes #961.

This is because the Z SELinux label assumes the data is not shared between containers and not being used by the host, as documented below:

If you use selinux you can add the z or Z options to modify the selinux label of the host file or directory being mounted into the container. This affects the file or directory on the host machine itself and can have consequences outside of the scope of Docker.

  • The z option indicates that the bind mount content is shared among multiple containers.
  • The Z option indicates that the bind mount content is private and unshared.

Use extreme caution with these options. Bind-mounting a system directory such as /home or /usr with the Z option renders your host machine inoperable and you may need to relabel the host machine files by hand.

Prior to this, we used the Z label, when we should have been using the z label.

Ensure that the volumes are not mounted as private, unshared volumes since we might mount with the host filesystem. This also fixes permissions issues with reading data from a mounted volume using a rootless container engine.
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner July 18, 2022 17:49
@Alexhuszagh
Copy link
Contributor Author

You can test the logic behind this with the following (use Fedora since it comes with SELinux enforced by default):

$ getenforce
Enforcing
# no label
$ podman run -it --rm -v "$PWD":"$PWD" -w "$PWD" ubuntu:20.04 bash -c "ls"
ls: cannot open directory '.': Permission denied
$ podman run -it --rm -v "$PWD":"$PWD":z -w "$PWD" ubuntu:20.04 bash -c "ls"
Cargo.lock  Cargo.toml  src  target
$ podman run -it --rm -v "$PWD":"$PWD":Z -w "$PWD" ubuntu:20.04 bash -c "ls"
Cargo.lock  Cargo.toml  src  target

Note that once the z or Z SELinux label is used on a directory, Podman or Docker will allow it to be read even if a label is not provided later.

Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 19, 2022

Build succeeded:

@bors bors bot merged commit b0e3b54 into cross-rs:main Jul 19, 2022
@Alexhuszagh Alexhuszagh deleted the selinux_labels branch July 19, 2022 22:44
@Emilgardis Emilgardis added this to the v0.3.0 milestone Aug 10, 2022
@lriesebos
Copy link

Hi, I was wondering, is it worth to have have a release that includes this fix? Like together with other fixes from master as v0.2.5 or if necessary as a patch on v0.2.4? This is a major blocker for Fedora systems and having it as a release would be helpful.

@lriesebos
Copy link

@Alexhuszagh ?

@Emilgardis
Copy link
Member

Sorry for the lack of response, I want to do this but haven't taken the time to do so. I'm not sure what else we should include in v0.2.5 but just this. I'll make the release tomorrow

@Emilgardis Emilgardis mentioned this pull request Feb 3, 2023
bors bot added a commit that referenced this pull request Feb 4, 2023
1201: Release v0.2.5 r=Emilgardis a=Emilgardis

See #962 (comment)

includes

ee3c972
fc3df66
33ee940
81c1e59
6a57d01

Co-authored-by: Emil Gardström <[email protected]>
Co-authored-by: Alex Huszagh <[email protected]>
Co-authored-by: Taiki Endo <[email protected]>
bors bot added a commit that referenced this pull request Feb 4, 2023
1201: Release v0.2.5 r=Emilgardis a=Emilgardis

See #962 (comment)

includes

ee3c972 #962
fc3df66 #1166
33ee940 in #997
81c1e59 #950
6a57d01 #1183

Co-authored-by: Emil Gardström <[email protected]>
Co-authored-by: Alex Huszagh <[email protected]>
Co-authored-by: Taiki Endo <[email protected]>
bors bot added a commit that referenced this pull request Feb 4, 2023
1201: Release v0.2.5 r=Emilgardis a=Emilgardis

See #962 (comment)

includes

ee3c972 #962
fc3df66 #1166
33ee940 in #997
81c1e59 #950
6a57d01 #1183

Co-authored-by: Emil Gardström <[email protected]>
Co-authored-by: Alex Huszagh <[email protected]>
Co-authored-by: Taiki Endo <[email protected]>
@lriesebos
Copy link

hi, @Emilgardis took me a while to circle back, but I just tested v0.2.5 and there seems to be a regression (i.e. aa59bf2 still works fine), but I can't reproduce it reliably though.

regardless, when it happens, a permission denied error appears again. looking at the file and generated podman command, it turns out that aa59bf2 also appends :z to cargo path dependencies while v0.2.5 does not. was that removed intentionally, or is that a potential regression?

@Emilgardis Emilgardis modified the milestones: v0.3.0, v0.2.5 Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix SELinux Labels
3 participants