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

overlay.d: add 07fix-selinux-labels overlay #3150

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

jbtrystram
Copy link
Contributor

/boot/efi and /sysroot dir and subfiles are unlabeled_t since
40.20240504.3.0.
This is likely due to a missing step in the OSBuild pipeline as this
started with coreos/fedora-coreos-tracker#1653.

This should be removed after the next barrier release, if the newly
produced images are fixed.

See coreos/fedora-coreos-tracker#1771
And coreos/fedora-coreos-tracker#1772

@travier
Copy link
Member

travier commented Sep 9, 2024

See https://docs.fedoraproject.org/en-US/fedora-silverblue/troubleshooting/#_running_restorecon for why you should never run restorecon -R on /sysroot.

It's also part of the reasons that we made /sysroot RO by default on Atomic Desktops. There should be more links in the change page.

@dustymabe
Copy link
Member

Probably going to need to make this more complex unfortunately: coreos/fedora-coreos-tracker#1772 (comment)

@travier
Copy link
Member

travier commented Sep 13, 2024

Let's focus on /boot first as that's what blocking bootupd. We can fix /sysroot afterwards.

find "$root_mount_point" -context '*:unlabeled_t:*' > "$unlabeled"

to_relabel=$(mktemp -d)
# filter out ostree repo objects
Copy link
Member

Choose a reason for hiding this comment

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

Instead of filtering, I would select only the files that we know we can safely relabel instead.

Copy link
Member

@travier travier Sep 17, 2024

Choose a reason for hiding this comment

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

Ideally we would call restorecon on those files instead of hardcoding a label in the script. But that might not work.

Copy link
Member

Choose a reason for hiding this comment

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

IIUC we can't just call restorecon without having everything mounted in the right place, which we could do. i.e. on a running system sudo restorecon -vr /var/ /etc/ /usr/ /boot/ but that doesn't really handle some of the unlabeled files under /sysroot/ or /osree/ for which there isn't really any policy, which is kind of why we are in the mess we are in now.

I was thinking we could just find all files under / with unlabeled_t and make them root_t but I think @jlebon thought that wouldn't be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would call restorecon on those files instead of hardcoding a label in the script. But that might not work.

there is no policy for files under /sysroot so that would not work

@jbtrystram
Copy link
Contributor Author

Here is what the root filesystem looks like after this script ran.
For comparison, this is extracted from a good filesystem
the only file i am curious about is /ostree/deploy/fedora-coreos/deploy/$COMMIT.0.origin

@jbtrystram jbtrystram marked this pull request as ready for review September 18, 2024 08:18
# Remove after the next barrier release

[Unit]
Description=Fix incorrect selinux labels under /boot and /sysroot
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Description=Fix incorrect selinux labels under /boot and /sysroot
Description=CoreOS Fix SELinux Labels

overlay.d/15fcos/usr/libexec/coreos-fix-labels Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/libexec/coreos-fix-labels Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/libexec/coreos-fix-labels Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/libexec/coreos-fix-labels Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/libexec/coreos-fix-labels Outdated Show resolved Hide resolved
overlay.d/15fcos/usr/libexec/coreos-fix-labels Outdated Show resolved Hide resolved
@dustymabe dustymabe changed the title overlay/15fcos: fix selinux labels in /boot and /sysroot overlay.d: add 07fix-selinux-labels overlay Sep 26, 2024
@dustymabe
Copy link
Member

I've updated this PR to fix existing FCOS and RHCOS systems and to include the tests from #3172 (which I'll now close in favor of this PR for those tests).

@dustymabe
Copy link
Member

I think I've tested this enough now that I'm comfortable with it merging if it passes review.

HuijingHei
HuijingHei previously approved these changes Sep 27, 2024
Copy link
Member

@HuijingHei HuijingHei left a comment

Choose a reason for hiding this comment

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

Nice and neat, overall LGTM.

Copy link
Contributor Author

@jbtrystram jbtrystram left a comment

Choose a reason for hiding this comment

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

Thanks @dustymabe for reworking that. Well documentend and easy to review !
I have just one concern about relabelling stuff in var

/sysroot/ostree/repo/{.lock,config} \
/sysroot/ostree/boot* \
-context '*:unlabeled_t:*';
find "/sysroot/" -maxdepth 7 -context '*:unlabeled_t:*' \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to go in the users' data in /sysroot/ostree/deploy/*/var directory. should we be concerned about relabelling files theres ?

Copy link
Member

Choose a reason for hiding this comment

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

added a -prune to forcefully skip the var dir

@dustymabe dustymabe force-pushed the fix-selinux-contexts branch 3 times, most recently from b1336af to 20c266b Compare September 27, 2024 13:38
@jbtrystram
Copy link
Contributor Author

I can't approved my own PR, but LGTM !

HuijingHei
HuijingHei previously approved these changes Sep 27, 2024
@dustymabe
Copy link
Member

I'll leave this open for a few more hours for further reviews.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this!

dustymabe and others added 4 commits September 27, 2024 17:25
The new name I think gives a better indication of what the test is
doing.
/boot/efi and /sysroot dir and subfiles are unlabeled_t since
40.20240504.3.0. This is likely due to some missing scaffolding
in the OSBuild software and definitions that we started using in
[1]. These issues [2] [3] were addressed in [4] for new image
builds, but we still need to fix upgrading systems, which we
do here in this migration script.

Note that we also fix a few files in /boot that were left
unlabeled by `rdcore` [5] while we are in here.

[1] coreos/fedora-coreos-tracker#1653.
[2] coreos/fedora-coreos-tracker#1771
[3] coreos/fedora-coreos-tracker#1772
[4] coreos/coreos-assembler#3885
[5] coreos/fedora-coreos-tracker#1770

Co-authored-by: Dusty Mabe <[email protected]>
This adds a unlabeled and mislabeled files test and also adds
code to the extended upgrade test to verify there aren't any
suprises there either.
On every boot, the `/var/mnt` directory *in the deployment root*
(i.e. not on top of any `/var` bind-mount/filesystem mount) would
get recreated and be unlabeled. After a lot of digging and busting out
systemtap, this turned out to be systemd doing this as part of switching
root as a temporary mount point.

In systemd v254+, this behaviour was changed to no longer require this
directory:

systemd/systemd@f2c1d49

For completeness, update the comment in this test to reflect these
findings.
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

All code review comments have been addressed.

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.

5 participants