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

[WIP] build_library: set correct SELinux contexts in final images #66

Closed
wants to merge 1 commit into from

Conversation

dongsupark
Copy link
Member

@dongsupark dongsupark commented May 19, 2020

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.

Copy link
Contributor

@margamanterola margamanterola left a 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.

@dongsupark dongsupark force-pushed the dongsu/selinux-relabel-alpha branch from 5c014bf to 4a88c2c Compare May 20, 2020 12:55
@dongsupark
Copy link
Member Author

Added comments in the script.

Copy link
Contributor

@margamanterola margamanterola left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Comment on lines 535 to 536
sudo umount "${root_fs_dir}/sys/fs/selinux"
sudo umount "${root_fs_dir}/sys"
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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 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

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

Copy link
Member Author

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.

@dongsupark dongsupark force-pushed the dongsu/selinux-relabel-alpha branch from 4a88c2c to 44244b2 Compare May 25, 2020 10:17
@pothos
Copy link
Member

pothos commented Jun 26, 2020

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.
@dongsupark dongsupark force-pushed the dongsu/selinux-relabel-alpha branch from 82a4414 to 1198151 Compare June 29, 2020 13:41
@dongsupark
Copy link
Member Author

Rebased

@pothos pothos changed the base branch from flatcar-master-alpha to main July 23, 2020 15:47
@vbatts
Copy link
Member

vbatts commented Sep 3, 2020

This looks ready. Just needing another quick review?

@dongsupark dongsupark changed the title build_library: set correct SELinux contexts in final images [WIP] build_library: set correct SELinux contexts in final images Sep 3, 2020
@dongsupark
Copy link
Member Author

dongsupark commented Sep 3, 2020

No, it is still work-in-progress, as flatcar-archive/coreos-overlay#347 is stuck with other issues.
Last time when @krnowak looked into the set of SELinux updates, it somehow caused SELinux relabeling issues during Jenkins CI tests.
Not sure what the reason was.

@dongsupark
Copy link
Member Author

Probably not needed, please see flatcar-archive/portage-stable#172 and flatcar-archive/coreos-overlay#1048 .

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.

5 participants