-
Notifications
You must be signed in to change notification settings - Fork 47
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
[WIP] build_library: set correct SELinux contexts in final images #66
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add comments on the script to explain why this is done and what is going on? Thanks.
5c014bf
to
4a88c2c
Compare
Added comments in the script. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
build_library/build_image_util.sh
Outdated
sudo umount "${root_fs_dir}/sys/fs/selinux" | ||
sudo umount "${root_fs_dir}/sys" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We unconditionally unmount /sys/fs/selinux
and /sys
here, while condtionally mounting them above. It leads me to two questions:
- Is it ever a case that
/sys
or/sys/fs/selinux
were already mounted? - Should unmounting be conditional too? So we unmount the filesystems only if we have just mounted them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ever a case that /sys or /sys/fs/selinux were already mounted?
In an ideal world, Flatcar SDK will be created from scratch, without /sys
inside the SDK environment being mounted. So we would not need to check for the mount, as you said.
However, there were some races around mountpoints or directories, especially it runs under Jenkins CI. (Never trust Jenkins about that!) I am afraid we cannot simply say the directories and mountpoints are always completely cleaned up. That's why I checked for the mountpoints, to be sure.
Should unmounting be conditional too? So we unmount the filesystems only if we have just mounted them.
That sounds like a reasonable idea.
There might be race conditions, as we run restorecon
for the whole rootfs, which could take a fair amount of time.
So how about checking for mountpoint like that?
if mountpoint -q "${root_fs_dir}/sys/fs/selinux"; then
sudo umount "${root_fs_dir}/sys/fs/selinux"
fi
if mountpoint -q "${root_fs_dir}/sys"; then
sudo umount "${root_fs_dir}/sys"
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should unmounting be conditional too? So we unmount the filesystems only if we have just mounted them.
That sounds like a reasonable idea.
There might be race conditions, as we runrestorecon
for the whole rootfs, which could take a fair amount of time.So how about checking for mountpoint like that?
if mountpoint -q "${root_fs_dir}/sys/fs/selinux"; then sudo umount "${root_fs_dir}/sys/fs/selinux" fi if mountpoint -q "${root_fs_dir}/sys"; then sudo umount "${root_fs_dir}/sys" fi
I was thinking more like following snippet below. But I suppose that if we have race conditions then all bets are off and stuff can break at any time.
local did_mount_sys=0, did_mount_sys_fs_selinux=0
if ! mountpoint -q "${root_fs_dir}/sys"; then
did_mount_sys=1
sudo mount -t sysfs none "${root_fs_dir}/sys"
fi
if ! mountpoint -q "${root_fs_dir}/sys/fs/selinux"; then
did_mount_sys_fs_selinux=1
sudo mount -t selinuxfs none "${root_fs_dir}/sys/fs/selinux"
fi
# do selinux business here
if [[ ${did_mount_sys_fs_selinux} -eq 1 ]]; then
sudo umount "${root_fs_dir}/sys/fs/selinux"
fi
if [[ ${did_mount_sys} -eq 1 ]]; then
sudo umount "${root_fs_dir}/sys"
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right.
Updated the PR as you suggested.
4a88c2c
to
44244b2
Compare
44244b2
to
82a4414
Compare
Needs to be rebased now to include the split SDK changes. |
We need to basically run `restorecon -R -v /` on all files in the final rootfs, before finalizing the image. Without doing that, nearly every file will have `unlabeled_t` as its context. Then with recent versions of selinux-base, some critical actions would not work correctly in SELinux enforcing mode, e.g., loading Kernel modules. To be able to run `restorecon`, `/sys/fs/selinux` has to be mounted. Without that, it will silently skip relabelling.
82a4414
to
1198151
Compare
Rebased |
This looks ready. Just needing another quick review? |
No, it is still work-in-progress, as flatcar-archive/coreos-overlay#347 is stuck with other issues. |
Probably not needed, please see flatcar-archive/portage-stable#172 and flatcar-archive/coreos-overlay#1048 . |
Work-in-progress: please do not merge
We need to basically run
restorecon -R -v /
on all files in the final rootfs, before finalizing the image.Without doing that, nearly every file will have
unlabeled_t
as its context.Then with recent versions of selinux-base, some critical actions would not work correctly in SELinux enforcing mode, e.g., loading Kernel modules.
To be able to run
restorecon
,/sys/fs/selinux
has to be mounted. Without that, it will silently skip relabelling.This PR should be merged together with flatcar-archive/portage-stable#66, flatcar-archive/coreos-overlay#347.